search

nickslick03 / messiah-meal-planner

1 stars 0 forks source link

Date and total calculations #32

Closed nickslick03 closed 1 month ago

nickslick03 commented 1 month ago

I created the timing and meal total functions with unit tests and hooked them to their respective components. I also made a mealIndex object that links the meal IDs to its index so the Array.find() function doesn't have to be called on the meal references to find its corresponding meal (check out implementation in the getMealTotal function in calculationEngine.ts). If you prefer this method of meal lookup, perhaps you'll want to refactor the code that uses .find()? Let me know what you think about that and the rest of the PR.

nickslick03 commented 1 month ago

Also, the weekday index starts on Monday in the code so far, but the calculation functions understand the week to start on Sunday since that's how the date object works in JS. I only needed to convert things twice on DayEditor.tsx, lines 129 and 133. Let me know if this is something you think is worth synchronizing.

cdrice26 commented 1 month ago

Ok, I mostly just added documentation and removed a few unused imports on DayEditor.tsx and I think we can merge. As far as restructuring to use mealIndex, I think that would be better in terms of speed as currently the conversion algorithm is O(n^2) which is not ideal, whereas indexing would bring this down to O(n). I've never used Array.reduce() before so it took some work to figure out what mealIndex is but it makes sense to me now and would allow direct indexing which would be nice. However, this would require an additional context for the combined meals and custom meals, and then an index associated with that. It's certainly doable, but probably not with this PR, so it's something I can look at in the next sprint. I do think switching the DayEditor to start on Sunday rather than Monday would eliminate some confusion. For now I just commented on those lines why we have the conversion and it helps, but switching to start on Sunday would be better yet - given that the userSelectedMealsObject uses instance variables for each weekday, I think the changes would be fairly localized to this component but I could be wrong - if it gets too complicated it's no big deal. Overall I'd say just go ahead and merge this PR and we can make code cleanup, including reindexing the days in DayEditor and removing .find(), in Sprint 5 along with the search feature and tutorial, but it's up to you, if there's something you want to squeeze in here yet, feel free to do so.

cdrice26 commented 1 month ago

Calculation engine looks very well done overall though!