Robert test branch #39
Conversation
|
|
||
| @GetMapping(value = "/retrieve/{userId}") | ||
| @ResponseBody | ||
| public List<PaymentDto> retrieve(@NonNull final @PathVariable String userId) { |
There was a problem hiding this comment.
What benefit does @NonNull provide here?
There was a problem hiding this comment.
I added the @nonnull annotation here to make sure that a NullPointerException would be thrown if this method is ever called without userId since userId is required for this method.
There was a problem hiding this comment.
I just tested this some more by making curl calls to the API on my terminal. It seems like even if I don't provide a userId the API just treats this as a userId with the value of EMPTYSTRING so I think you are right and this check is not needed. I think the only way userId will be null is if someone specifically passes in an encoded version of null and when I tested that, it was causing the API to throw a BadRequest exception so I think this is already handled.
| final List<Integer> ids = IntStream.range(1, 10).boxed().collect(Collectors.toList()); | ||
|
|
||
| this.payments = ids.stream().map(id -> createPaymentDto(Integer.toString(id))) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
ids variable is unnecessary since it's only used once. You can combine these statements together.
this.payments = IntStream.range(1, 10).boxed()
.map(id -> createPaymentDto(Integer.toString(id)))
.collect(Collectors.toList())
There was a problem hiding this comment.
Good call out. I have refactored the code and fixed this to removed the redundant variable.
| @@ -48,7 +48,7 @@ public <T extends Collection<Payment>> List<PaymentDto> convertPayments(final T | |||
| public PaymentDto convertPayment(final Payment payment){ | |||
| final String cardNumber = payment.getCardNumber(); | |||
| final PaymentDto dto = mapperFacade.map(payment, PaymentDto.class); | |||
| dto.setCardNumberLast4(cardNumber.substring(cardNumber.length() - 5)); | |||
| dto.setCardNumberLast4(cardNumber.substring(cardNumber.length() - 4)); | |||
There was a problem hiding this comment.
Good job catching this bug! 👍🏻
Details
Revision 2:
Refactoring PaymentController and PaymentControllerTest code to revmove redundant variables.
Why Are We Making This Change?
What Is Changing In This Review?
How Was This Tested?