feat(task-2): Create the "Statistics" Feature #34
Conversation
margarita-pashnina
requested review from
BagumEpmak,
IgorGursky,
Melancholic,
algorithm108 and
trioletas
| int completedTodos, | ||
| int pendingTodos, | ||
| Map<String, Integer> userStats, | ||
| Optional<Map<String, List<StatisticsTodo>>> todos) {} |
There was a problem hiding this comment.
Unusual formatting. If you use IDEA you can press Ctrl+Llt+L or check "Reformat code" in commit options.
There was a problem hiding this comment.
Spotless check on build fails otherwise, maybe it's supposed to look like this
BagumEpmak
left a comment
BagumEpmak
left a comment
There was a problem hiding this comment.
Please fix these issues first:
- Response for summary
response for summary should not include "todos"
- Swagger definition
Current swagger page:
I would expect something like this:
fixed |
| return ResponseEntity.ok(statisticsService.getStatisticsSummary(from, to, format)); | ||
| } | ||
|
|
||
| @ExceptionHandler(ConstraintViolationException.class) |
There was a problem hiding this comment.
@margarita-pashnina I would suggest to extract exception handlers to a separate common class. As these ConstraintViolationException can be thrown everywhere across the app. Imagine you have 10 controllers, but the behaviour shall be the same for all throws, I assume
| public class StatisticsFormatValidator implements ConstraintValidator<StatisticsFormat, String> { | ||
| @Override | ||
| public boolean isValid(String value, ConstraintValidatorContext context) { | ||
| return value.equals("summary") || value.equals("detailed"); |
There was a problem hiding this comment.
A small trick. It's better to compare as "summary".equals(value), so that there never is NullPointerException thrown
There was a problem hiding this comment.
good to know, thanks, fixed
|
|
||
| Map<String, Integer> userStats = getUserStats(result); | ||
|
|
||
| if (format.equals("summary")) { |
There was a problem hiding this comment.
Another idea, how about converting format from plain String to an ENUM? As it looks like only known type of formats are supported and API consumer cannot pass something else in a format value.
| .and(countTodosPerUser) | ||
| .as("userStats"); | ||
|
|
||
| FacetOperation detailsFacet = |
There was a problem hiding this comment.
Here I would split this method into two, for summary and detailed data to be returned. If I understood correctly, based on format type we return only one of the facetOperation results, mething that those initializations can be done and hidden WITHIN an IF or in a separate methods
No description provided.