getify / You-Dont-Know-JS

A book series on JavaScript. @YDKJS on twitter.
Other
179.24k stars 33.48k forks source link

Get Started - Appendix B: Bug in Proposed Solution #1825

Closed ericghara closed 1 year ago

ericghara commented 1 year ago

Please type "I already searched for this issue": I already searched for this issue"

Edition: 2nd

Book Title: Get Started

Chapter: Appendix B

Section Title: Suggested Solutions

Problem: The scheduleMeeting function returns an incorrect result for the following input: scheduleMeeting("7:30", 92*60+30). The function returns true, when the expected result is false.

This input causes the meetingEnd calculated by scheduleMeeting to be "100:00". By Lexicographic comparison "100:00" is less than "17:45" (dayEnd).

I recommend checking that meetingEnd.length == 5 or validating that durationMinutes <= 24*60. I'd be glad to to submit a PR, but wanted to open a discussion first.

P.S. I enjoyed the book, thanks.

getify commented 1 year ago

I don't think this is an issue that we will fix. Here's why: the real problem here is not the logic suggested in the solution, but that the setup of the problem is somewhat under-specified (it relies on what I think are reasonable, but non-explicit, assumptions).

IOW, there is an assumption that the function is only used with inputs that stay within the same day. The solution doesn't handle validation for inputs outside of that range.

I assumed from the setup that it would be clear that we're only using timestamps on a same day, and that the duration of the meeting cannot go past the end of the day. Furthermore, the wording in the text says "...if the meeting falls entirely within the work day." If a meeting falls within a work-day (in the traditional definition thereof), it definitely has to fall within a regular 24-hr day (ending before midnight). I made no mention of handling "work days" that span midnight, or any of a variety of different complexities.

Staying within a single 24-hr day would be the only way that sorting with 24-hr timestamps would be possible/reliable. I was deliberately designing this to restrict the "domain" of the problem to make it simpler.

So if I were going to fix anything, I might have added some sentence to the setup text for the problem earlier in the appendix, which indicated that the function can expect inputs within the relevant range for the day and that it did not need to cover any other sorts of inputs.

In my mind, this is the same kind of assumption as not allowing/handling negative values for meeting duration (there's no such thing as a meeting with negative duration). The code doesn't make any attempt to validate that because it's an assumed pre-condition. Similarly, there's an assumed pre-condition that start+duration won't go past midnight. I maybe should have said that explicitly, but I assumed it would be obvious enough.

ericghara commented 1 year ago

Thanks, I appreciate your reply. It's ultimately a judgement call and I wanted to bring it to your attention.

getify commented 1 year ago

Appreciate you bringing it up.

If the book was still in draft or even recently published, I might have added a phrase/sentence to the text. But since the book was published more than 3 years ago, I think it's probably not an important enough change to warrant issuing a correction.

Cheers!