Dagim H.#4
Conversation
composix
left a comment
composix
left a comment
There was a problem hiding this comment.
This is a nice and functional implementation of the application. It is clear that you explored several different approaches that Java provides, which is good for learning. The main improvement would be to make more deliberate choices and apply them consistently throughout the codebase, resulting in a cleaner and more maintainable way of working.
|
|
||
| import lombok.Getter; | ||
| import lombok.Setter; | ||
|
|
There was a problem hiding this comment.
This is a solid approach for object deserialization on DTOs for API calls. However, the use of @Setter suggests that the DTO is mutable. Using the record feature of Java instead could be a good idea here to make the immutability more clear. Jackson works very well with Java records.
| import lombok.Getter; | ||
| import lombok.NoArgsConstructor; | ||
| import lombok.Setter; | ||
|
|
There was a problem hiding this comment.
This model mixes constructor style injection with setter injection. It is better to make a choice here: either a @Setter or an @AllArgsConstructor. In this case, the dto is immutable so constructor injection would be the best choice. Then the @Setter and @NoArgsContructor can be removed.
| import org.springframework.web.bind.annotation.RestControllerAdvice; | ||
|
|
||
| import java.util.Map; | ||
|
|
There was a problem hiding this comment.
Nice GlobalExceptionHandler. Also the 404 for the UserStatistics service could be added here.
| "uniqueArtistsStreamed", | ||
| "favoriteGenre", | ||
| "totalListeningTimeSeconds" | ||
| }) |
There was a problem hiding this comment.
Here again, choose either constructor- or setter dependency injection style.
| )); | ||
|
|
||
| }, trackId); | ||
| } |
There was a problem hiding this comment.
Nice choice choosing a record here. For now, since there is no update logic you could make the records immutable using the final keyword on its fields.
| @GetMapping("/{id}/statistics") | ||
| public ResponseEntity<?> getStats(@PathVariable int id) { | ||
| UserStatisticsResponse stats = service.getStats(id); | ||
|
|
There was a problem hiding this comment.
This null check can be avoided nicely by using the GlobalExceptionHandler to produce a 404.
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
|
||
| public class UserStatisticsResponseTest { | ||
|
|
There was a problem hiding this comment.
The models in this application are very straightforward and do not contain any logic to test. Therefore, unit tests for them can be avoided. That said, writing unit tests is generally a good practice, of course.
| @@ -0,0 +1,4 @@ | |||
| package net.hackyourfuture.backend.week6.postify.repository; | |||
|
|
|||
| @@ -0,0 +1,4 @@ | |||
| package net.hackyourfuture.backend.week6.postify.repository; | |||
|
|
|||
| @@ -0,0 +1,4 @@ | |||
| package net.hackyourfuture.backend.week6.postify.service; | |||
|
|
|||
There was a problem hiding this comment.
In general, the service layer is the best place to unit test business logic. For this application, I would prioritize tests around the services rather than the controller tests that are currently present, since the services contain the actual behavior we want to verify.
2 participants
Completed Task 1- Implemented /users/{id}/statistics endpoint that returns user listening statistics using SQL joins, with 404 handling and integration tests.
And Task 2-Implemented /tracks/{id}/lyrics endpoint that fetches lyrics via RestClient, handles missing tracks/lyrics with 404 errors & full integration tests.