Greg Vella Candidate Assessment #38

gandveep · 2022-12-02T18:41:12Z

Screenprints demonstrating functionality at end of comments. I added a user with 3 payments, one with 2 payments, and one with 1 payment.

My Comments:

I know this is a somewhat hypothetical coding exercise, but here are some comments/assumptions/observations:

The ma.glasnost.orika classes are not compatible with the current Java JDK17 version. I thought the missing jars and "dependency not satisfied" startup issues were part of the exercise to test debugging skills. But then I downloaded JDK11 and base project started without issue. Just a suggestion to make the JDK11 requirement a "must be at this level". I thought it was just a minimum level required.
I added the payment classes per the checklist. However, an alternate implementation could have been adding the payment endpoint as part of the User classes: /user/{user-id}/payment. And I would have grabbed the payment data through the one-many relationship from the User JPA entity.
I used the paging strategy from the Payment repository rather than just a straight "findByUserId" method. Even though I wouldn't think a customer would have that many credit cards (but anything is possible). :)
I reused the UserValidator class for verifying userId since any potential validation changes to userId would need to cascade to all use cases.
I only returned data from the Payment table for the User based on the context of Expected statement: "return all data for a given User". In real life, I would have confirmed if only payment data is required or if also needed user/address information as part of a consolidated json response.
In real life, I would add a secondary db index on the userid in the Payment table for performance.
Another suggestion or alternate implementation would be adding a standard error block json structure as part of the response (so there is a data block, then an error block). Would allow for returning a 200 response but also provide error/status information if needed (in the case of
payment data not found for user, userid not found, etc.).
I kept my branch as "main" for this exercise. However, I would normally create a feature branch from "main"(or the required base branch) and then PR back into the main branch. All depends on the branch strategy used for a project.

Sorry for the long-winded comments but wanted to list out the items I thought about during the exercise. Thank you!

gandveep · 2022-12-02T18:43:59Z

Sorry, missed one comment in initial Post:

The unique constraint on the Payment table could be relaxed to a compound: userid/cardnumber. I thought of the case of a husband/wife or parents/children having separate login accounts, but might be using joint credit cards. Just something I thought about when testing. It's more secure this way(with uniqueness on card number), but might cause consternation to end customers.

tlynema-bravolt · 2022-12-02T21:29:57Z

src/main/java/com/bravo/user/model/filter/PaymentFilter.java

+@Builder
+@AllArgsConstructor
+@NoArgsConstructor


Combinine Builders and Constructors allows for 3 different ways to set the value, the builder, the constructor, and the setter. Are all of these needed?

I'm only using the builder for this class, so really not needed. Habit from my last project of adding all 3 of these.

tlynema-bravolt · 2022-12-02T21:32:18Z

src/main/java/com/bravo/user/service/PaymentService.java

+@Service
+public class PaymentService {
+
+    private static final Logger LOGGER = LoggerFactory.getLogger(PaymentService.class);


I've seen this in many different ways so would like your thoughts: here, LOGGER is all caps. Is that in line with established naming conventions?

Existing UserService.class used this format, so I kept it consistent for the PaymentService. I've used the SLF4J annotation previously (so the reference is log.info, etc.). If no prior precedent in existing code, I would have likely named it log.

tlynema-bravolt · 2022-12-02T21:33:02Z

src/main/java/com/bravo/user/service/PaymentService.java

+        final PaymentSpecification specification = new PaymentSpecification(filter);
+        final Page<Payment> paymentPage = paymentRepository.findAll(specification, pageRequest);
+        final List<PaymentDto> payments = resourceMapper.convertPayments(paymentPage.getContent());
+        LOGGER.info("found {} payments(s)", payments.size());


Does this enough value to be in included in every call?

No, in this case, doesn't really add any significant value for production debugging, verification, etc. Could be more useful if it output userid and payment list size. Would not output any payment data to logs (due to data sensitivity). So could be removed or enhanced, depending on what's needed.

tlynema-bravolt · 2022-12-02T21:33:52Z

src/main/resources/data.sql

@@ -648,3 +648,11 @@ insert into address (id, user_id, line1, line2, city, state, zip) values
 ('42f33d30-f3f8-4743-a94e-4db11fdb747d', '008a4215-0b1d-445e-b655-a964039cbb5a', '412 Maple St', null, 'Dowagiac', 'Michigan', '49047'),
 ('579872ec-46f8-46b5-b809-d0724d965f0e', '00963d9b-f884-485e-9455-fcf30c6ac379', '237 Mountain Ter', 'Apt 10', 'Odenville', 'Alabama', '35120'),
 ('95a983d0-ba0e-4f30-afb6-667d4724b253', '00963d9b-f884-485e-9455-fcf30c6ac379', '107 Annettes Ct', null, 'Aydlett', 'North Carolina', '27916');
+
+insert into payment (id, user_id, card_number, expiry_month, expiry_year) values
+('43f33d30-f3f8-4743-a94e-4db11fdb747d', '008a4215-0b1d-445e-b655-a964039cbb5a', '1234567890123456', 12, 2024),


The last 4 digits is repeated in all tests, this may lead to good code coverage and not great scenario coverage.

Agreed. Should have randomized the trailing digits instead of the leading digits to make unique card numbers.

tlynema-bravolt · 2022-12-02T21:35:33Z

src/test/java/com/bravo/user/controller/PaymentControllerTest.java

+        final List<Integer> ids = IntStream
+                .range(1, 10)
+                .boxed()
+                .collect(Collectors.toList());


Is ids mutable here? Looking at this, it isn't the most readable code, it does work of course. Would like to know if you see this as better than a traditional for loop.

Yes, values in ids variable are mutable. The ids reference is immutable. I normally prefer for loops over this type of statement as I find it easier to understand. However, I was trying to keep the test classes consistent (as this construct was used in existing classes).

tlynema-bravolt · 2022-12-02T21:36:48Z

src/test/java/com/bravo/user/controller/PaymentControllerTest.java

+    @Test
+    void getRetrieveWithUserId() throws Exception {
+        when(paymentService
+                .retrievePayments(anyString(), any(PageRequest.class), any(HttpServletResponse.class)))


Using anyString here matches everything. Does being that permission take away from the value of this test or is it testing something else?

I would consider controller tests to be more about ensuring the integration of the components. The service tests are more geared to test the business logic. So in this example, I don't feel it diminishes the value of the test. But I'm always open to other interpretations or better ways to do things. :)

tlynema-bravolt · 2022-12-02T21:41:15Z

Thanks for taking the time to work on this @gandveep . We put comments on code reviews to see how candidates interact with differing viewpoints. Interested to see how you answer.

tlynema-bravolt · 2022-12-02T21:43:44Z

Thanks for the note on orika. Looks like it is a dead project orika-mapper/orika#377.

gandveep added 2 commits December 2, 2022 09:26

Code commit for User payment items

9350d06

Readd newlines to files

6e33af8

tlynema-bravolt reviewed Dec 2, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Greg Vella Candidate Assessment #38

Greg Vella Candidate Assessment #38

gandveep commented Dec 2, 2022

gandveep commented Dec 2, 2022

tlynema-bravolt Dec 2, 2022

gandveep Dec 3, 2022

tlynema-bravolt Dec 2, 2022

gandveep Dec 3, 2022

tlynema-bravolt Dec 2, 2022

gandveep Dec 3, 2022

tlynema-bravolt Dec 2, 2022 •

edited

Loading

gandveep Dec 3, 2022

tlynema-bravolt Dec 2, 2022

gandveep Dec 3, 2022

tlynema-bravolt Dec 2, 2022

gandveep Dec 3, 2022

tlynema-bravolt commented Dec 2, 2022

tlynema-bravolt commented Dec 2, 2022

Greg Vella Candidate Assessment #38

Are you sure you want to change the base?

Greg Vella Candidate Assessment #38

Conversation

gandveep commented Dec 2, 2022

gandveep commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlynema-bravolt Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlynema-bravolt commented Dec 2, 2022

tlynema-bravolt commented Dec 2, 2022

tlynema-bravolt Dec 2, 2022 •

edited

Loading