Fix: Optimize course times display

[rm-yakovenko]

#237

[thdebay]

I'll take more time to review your code more in depth. From what I've seen, it works great and you've covered all the use cases.

I think we could tweak your algorithm a bit so that a course that occurs several days a week, always at the same time would always be "grouped" according to the time first : Mon, Tue, Thu 10:00 - 12:00 (maybe put a check to see it all times are equal at the start of the method, and then if the check fails, the behavior you've proposed is perfect).

[thdebay]

Also, it's important to check if $courseTimes is null otherwise it will throw an error when at foreach()

[rm-yakovenko]

I'll take more time to review your code more in depth. From what I've seen, it works great and you've covered all the use cases.

Sure, I'm open to any feedback or questions.

I think we could tweak your algorithm a bit so that a course that occurs several days a week, always at the same time would always be "grouped" according to the time first : Mon, Tue, Thu 10:00 - 12:00 (maybe put a check to see it all times are equal at the start of the method, and then if the check fails, the behavior you've proposed is perfect).

I think "grouping by time" will make the code much simpler, as there is no need to find sequences of days.

How should it work in this case?
https://github.com/academico-sis/academico/blob/oss/tests/Unit/CourseTest.php#L187-L208

Also, it's important to check if $courseTimes is null otherwise it will throw an error when at foreach()

Fixed, I totally forgot about this case.

[thdebay]

How should it work in this case? https://github.com/academico-sis/academico/blob/oss/tests/Unit/CourseTest.php#L187-L208

I would say that it should stay exactly as you suggested. Unless all the course times are exactly the same throughout the week, we apply your algorithm unchanged. What do you think?

[rm-yakovenko]

How should it work in this case? https://github.com/academico-sis/academico/blob/oss/tests/Unit/CourseTest.php#L187-L208

I would say that it should stay exactly as you suggested. Unless all the course times are exactly the same throughout the week, we apply your algorithm unchanged. What do you think?

Honestly, I don't know ;) It depends on the UX and what's best for the users.

Having Mon, Tue, Thu 10:00 - 12:00 leads to simpler code and will be easier to maintain rm-yakovenko@87b9d09 .

I'm still struggling with this test https://github.com/academico-sis/academico/blob/oss/tests/Unit/CourseTest.php#L187-L208:

Failed asserting that two strings are identical.
Expected :'Mon 10:00 AM - 11:00 AM / 11:30 AM - 12:45 PM'
Actual   :'Mon 10:00 AM - 11:00 AM | Mon 11:30 AM - 12:45 PM'
<Click to see difference>

 /var/www/app/tests/Unit/CourseTest.php:207