Refactor HandleClickInTimeLine to handle edge cases & Add Unit Tests #78
Conversation
| @@ -0,0 +1,666 @@ | |||
| import "@testing-library/jest-dom"; | |||
There was a problem hiding this comment.
Please checkout this unit test file as well :D
There was a problem hiding this comment.
The test looks fine overall but there are some duplicate cases that looks like they could be eliminated. I see that there are a few foundational cases (clicking in between, clicking in, clicking before, clicking after an event, handling overflow events, etc) but it looks like the tests cover these for a different N number of events (2-5 events). If the cases are handled the same way regardless of N then it's probably better to have less tests for the same coverage.
There was a problem hiding this comment.
Hi Chris, thank you for your comments! I was just thinking it'll be better to test the binary search algorithm with different array length as well as different search positions. (array in this case is displayItems) Can you tell me about your thoughts? @cthamrun
| @@ -21,9 +24,33 @@ export default function OneHourInterval(props) { | |||
| }), | |||
| }); | |||
|
|
|||
| // Given a id of a div and an mouse click event | |||
There was a problem hiding this comment.
Nit: "Given an id of a div and a mouse click event"
There was a problem hiding this comment.
Thank you! Addressed :D
| }); | ||
| }); | ||
|
|
||
| test("Case 4: One travelobject overflows from the previous day and clickPoint is in the rest of the day", () => { |
There was a problem hiding this comment.
If I'm understanding this correctly, maybe this should be clarified as "clickPoint is in the part of the day after overflow event"
There was a problem hiding this comment.
Good suggestion! I'll rename the test like your suggestion.
| }); | ||
| }); | ||
|
|
||
| test("Case 5: One travelobject overflows to the next day and clickPoint is in the rest of the day", () => { |
There was a problem hiding this comment.
If I'm understanding this correctly, maybe this should be clarified as "clickPoint is in the part of the day before overflow event"
There was a problem hiding this comment.
Good suggestion! I'll rename the test like your suggestion.
| @@ -0,0 +1,666 @@ | |||
| import "@testing-library/jest-dom"; | |||
There was a problem hiding this comment.
The test looks fine overall but there are some duplicate cases that looks like they could be eliminated. I see that there are a few foundational cases (clicking in between, clicking in, clicking before, clicking after an event, handling overflow events, etc) but it looks like the tests cover these for a different N number of events (2-5 events). If the cases are handled the same way regardless of N then it's probably better to have less tests for the same coverage.
handle click in Timelinefeature, I realize the current code can't handle cases where one 30-min div has a small TravelObject in the middle of the div.For example:
If user clicks in part A or part B of the div, the current code doesn't know which part is clicked and just return a random one A or B.
Before, we thought that using a function which looks for overlapping between the clicked div and a array of free Timeslot would help, but it only helps in cases like this (meaning overlapping function only helps deal with cases where a 30-min div has only one free timeslot):
Thus, this PR:
OneHourInterval)getFreeTimeSlotandsearchForOverlappingPointfunctionHandleClickedTimePoint)(1) and (2) help reduce the time complexity of the
handle click in Timelinefeature from O(n) to O(logn) (where n is the number of displayItems), since we no longer need to iterate through displayItems to get the free timeslots anymore.