[bravo-0013] Implement new payment API to receive payments by user id #25
Conversation
…nal fixes, adding mapstruct to pom, cover by tests
|
I want to start by saying that we are not expecting any additional commits. Feel free to make them if you wish, but that's not an expectation that we have. With that said I will go through your changes and leave technical comments on them. |
| <mysql.connector.version>8.0.24</mysql.connector.version> | ||
| <orika.core.version>1.5.4</orika.core.version> | ||
| <springfox.swagger2.version>2.5.0</springfox.swagger2.version> | ||
| <springfox.swagger.ui.version>2.5.0</springfox.swagger.ui.version> |
| @ResponseStatus(value = HttpStatus.NOT_FOUND) | ||
| public ErrorDto handlePaymentNotFoundException(final PaymentNotFoundException exception){ | ||
| log.info("Payments not found, details: {}", exception.getMessage()); | ||
| var errorDto = new ErrorDto(); |
There was a problem hiding this comment.
errorDto could be final
| import org.mapstruct.Mapping; | ||
| import org.mapstruct.Named; | ||
|
|
||
| @Mapper(componentModel = "spring") |
There was a problem hiding this comment.
Can you explain what componentModel = "spring" means?
There was a problem hiding this comment.
@wheelerswebservices it means construct mapper as spring bean. Because mapper has different construction types, using factory, spring beans etc.
| return payments.stream().map(this::convertPayment).collect(Collectors.toList()); | ||
| } | ||
|
|
||
| public PaymentDto convertPayment(final Payment payment){ |
There was a problem hiding this comment.
Can you elaborate on why the new PaymentMapper class is superior to these methods within the existing ResourceMapper class?
There was a problem hiding this comment.
@wheelerswebservices because mapperFacade, which is used here is not the flexible one from SOLID principles perpective.
First of all, if you would have 10 dto's which you need to map from the entity its not a good idea to create 20 methods inside ResourseMapper class.
Second, is that you need to implement all of the extra mapping, or when field names are different. In the Mapstruct its simple, you only need an interface, and specify everything only using annotations. Its very useful and simple to understand. Its flexible when you need some extra mappings, for example as was for card mask. So everything for this mapping is located only in 1 Java interface.
Thats why mapstruct is so popular today.
You could read all the details below: https://www.baeldung.com/java-performance-mapping-frameworks
| private static final String MESSAGE = "Payment not found for user with id {0}"; | ||
|
|
||
| public PaymentNotFoundException(String userId) { | ||
| super(MessageFormat.format(MESSAGE, userId)); |
There was a problem hiding this comment.
- Why did you use
MessageFormat.formatinstead ofString.format? - Do you think this
MESSAGEvariable is really necessary as it's only used in 1 place?
There was a problem hiding this comment.
@wheelerswebservices regarding first: I don't think this is really matters
second: this exception is only related to Payment not found flow, so there is not reason to extract this message somewhere else.
|
|
||
| @Data | ||
| @Builder | ||
| @AllArgsConstructor | ||
| @NoArgsConstructor | ||
| public class PaymentDto { |
There was a problem hiding this comment.
Why annotate with Builder, AllArgsConstructor, and NoArgsConstructor if they are not all being used?
There was a problem hiding this comment.
Builder used to simplify construction for testing. Since we use Builder, NoArgs and AllArgs constrcustors is required for MapStruct.
Ticket bravo-0013
What was changed:
Why did I decide to use userId as a Request Parameter?
For me it looks weird we would have a path variable, since then the endpoint should look like /users/{userId}/payments.
As was mentioned in the ticket, we need to use PaymentController, so the best practices would be using userId as request param.
Sometimes it depends on what we would have in the future.
Also there was a couple of things:
We could fetch payments by creating new repository interface, as I've done, but on the other hand the implementation could be: