add a reporting endpoint for the trend of total sales amount #2
Conversation
| .orElse(LocalDateTime.MIN); | ||
|
|
||
| // Add in data for all orders to the SimpleRegression | ||
| customerOrderDao.getAllOrders().forEach(order -> { |
There was a problem hiding this comment.
You are fetching the orders all over again, but it is already done on line 28. It would be better fetch the orders once and store it into a variable and use that to perform your logic.
| public String saleAmountTrend () { | ||
| SimpleRegression r = new SimpleRegression(); | ||
|
|
||
| LocalDateTime tmp = customerOrderDao.getAllOrders().stream() |
There was a problem hiding this comment.
We should be checking if the query returns nothing, otherwise this would result in the wrong trend being returned. The orElse should never be reached as the only situation it can happen is if the result set is empty or we have missing data, both which are problematic.
Also, can we rename the variable to something a little more descriptive? It would clarify what this block of code is doing
| * Returns whether the total sales data is trending up or down | ||
| */ | ||
| public String saleAmountTrend () { | ||
| SimpleRegression r = new SimpleRegression(); |
There was a problem hiding this comment.
nit: this could be placed below the LocalDateTime block as it is not used until line 34. Can become an unnecessary creation if for whatever reason the code after this fails. Also lets go with a more descriptive variable name.
| // down, positive is trending up | ||
| Double result = r.getSlope(); | ||
| if (result < 0) { | ||
| return "down"; |
There was a problem hiding this comment.
Can we store these return values as static strings at the top of the class?
| * Analyzes trends in sales data | ||
| */ | ||
| @Component | ||
| public class TrendAnalyzer { |
There was a problem hiding this comment.
Non PR Comment: This could be made a PR comment, but I am choosing to discuss it here as I don't know the patterns established. I don't believe this class should be calling the customerOrderDao directly. I believe the caller should provide the data into the saleAmountTrend method. Something like ->
List<CustomerOrder> customerOrders = customerOrderDao.getAllOrders();
trendAnalyzer.saleAmountTrend(customerOrders)
This decouples the 2 components and allows TrendAnalyzer to be much more testable as no mocks are needed. It also allows saleAmountTrend to possibly be a util function if we ever genericize the type of data being passed. This further allows TrendAnalyzer to be a generic object instead of a bean, and therefor not needing to be managed by spring.
| * Returns the trend of the total sales data | ||
| */ | ||
| @RequestMapping(value = "/trend", method = RequestMethod.GET) | ||
| public String trend () { |
There was a problem hiding this comment.
nit: can we add a verb to this like getTrend?
No description provided.