Note that this commit will also disable daily grouping for datasets with more than 30 data points. This will also smartly do the grouping by month when grouping a full year.
- 1 for Useless assignment to local variable
const options = ["weekly", "monthly"];
In general – a real good extracting of logic, it makes things clearer .
It’s a bit confusing that this function takes
grouping as an argument and at the same time calculates
grouping value depending on data.
Probably the intention was to let a caller disable automatic grouping resolution by passing explicit
grouping value? If so, it looks like this code would overwrite the
grouping argument anyway. Probably this should be fixed.
I would go even further though, but I’m not insisting on this. I feel like this function is trying to be too smart, this implementation can be considered as a violation of the single responsibility design principle. This function
- chooses an appropriate grouping
- collapses data using this grouping
The first task can be moved into a separate function with an appropriate name which will make the code clearer.
const date = moment(d.x, "YYYY-MM-DD");
This seems to share some code with
_unitForDatapoints above. Could it be DRYed up?
It mostly share the constants, so yes maybe I could use constants