Salem B.#5
Conversation
composix
left a comment
composix
left a comment
There was a problem hiding this comment.
Good working application that functions as intended. One area for improvement is error handling. At the moment, some technical errors appear to result in a 404 Not Found, while unexpected server-side errors should generally return a 500 Internal Server Error. This distinction makes the API easier to understand for clients and helps with troubleshooting issues in production.
Consider adding proper logging for unexpected errors and implementing a global error handler, for example with @ControllerAdvice, to centralize error handling. This keeps the controllers focused on the happy flow and makes the application easier to maintain and debug.
|
|
||
| @Configuration | ||
| public class RestClientConfig { | ||
|
|
There was a problem hiding this comment.
Consider placing more of the static configuration for this RestClient in one central setup location. For example, the baseUrl could be configured once when creating the client, instead of being repeated or handled separately in the individual calls.
| import org.springframework.web.bind.annotation.PathVariable; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
| import org.springframework.web.bind.annotation.RestController; | ||
|
|
There was a problem hiding this comment.
Mooi gebruik van @AllArgsConstructor voor constructor injection. Eventueel zou je hier ook @requiredargsconstructor kunnen gebruiken, omdat die beter aansluit wanneer alleen final dependencies geïnjecteerd hoeven te worden.
| import lombok.AllArgsConstructor; | ||
| import lombok.Getter; | ||
| import lombok.Setter; | ||
|
|
There was a problem hiding this comment.
Using @Setter together with @AllArgsConstructor mixes mutable and constructor-based initialization styles. Since this DTO appears to be intended as immutable, I would remove @Setter and rely on constructor-based initialization instead.
| import lombok.AllArgsConstructor; | ||
| import lombok.Getter; | ||
|
|
||
|
|
There was a problem hiding this comment.
Nice work! I like how the chosen annotations make the intended immutability of this DTO clear.
| null // from external API | ||
| ), | ||
| trackId | ||
| ); |
There was a problem hiding this comment.
This catches all exceptions and returns null, which may hide real production issues such as SQL errors or database/connectivity problems. Consider catching only EmptyResultDataAccessException for the “not found” case and letting unexpected errors propagate or be logged.
| trackId | ||
| ); | ||
| } catch (Exception e) { | ||
| return null; |
There was a problem hiding this comment.
Returning null is prone to NullPointerExceptions in the calling code. Consider using Java’s Optional to make the “not found” case explicit, or even better, throw a domain-specific TrackNotFoundException.
| try { | ||
| String favoriteGenre; | ||
| try { | ||
| favoriteGenre = jdbcTemplate.queryForObject(genreSql, String.class, id); |
There was a problem hiding this comment.
The repository currently contains presentation logic by returning "None" as a fallback value. Consider handling this default value in the service or response layer instead.
| if (favoriteGenre == null) { | ||
| favoriteGenre = "None"; | ||
| } | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Catch only the exception you expect here.
| ); | ||
|
|
||
| } catch (Exception e) { | ||
| return null; |
There was a problem hiding this comment.
Catch only the exception you expect here.
| "I'm doing good, I'm on some new shi..." | ||
| ); | ||
|
|
||
|
|
There was a problem hiding this comment.
Good mocking of the TrackService, but now the TrackService itself has no test coverage yet. So you should add a TrackServiceTest too.
2 participants
No description provided.