getify / You-Dont-Know-JS

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

Update apB.md #1696

Closed sakastierov closed 4 years ago

sakastierov commented 4 years ago

I already searched for this issue

Edition: 2nd

Book Title: Get Started

Chapter: Appendix B: Practice, Practice, Practice!

Section Title: Suggested Solutions

Topic: Suggested solution for "Comparisons" (Pillar 3) practice Suggested solution for "Closure" (Pillar 1) practice

@getify, It is really awesome practice samples! Despite the fact, that I have been writing JS-code for quite a long term, It took me a lot of time to solve them without tips ๐Ÿ˜… They make me think for more elegant solutions!

I carefully checked my solution with yours by examining your source code and came across a few code style shortcomings Which, in my opinion, should be fixed to improve code readability

  1. Suggested solution for "Comparisons" (Pillar 3) practice The simpler case typeof meetingStartHour != "string" || (...) should be handled first to avoid over-nesting Makes sense?

  2. Suggested solution for "Closure" (Pillar 1) practice Statement else seems like just redundant as for me

I apologize in advance for my excessive pedantry in the scope of code style

Taking this opportunity to express my deep gratitude to the author for a series of these books!

getify commented 4 years ago

The simpler case typeof meetingStartHour != "string" || (...) should be handled first to avoid over-nesting Makes sense?

That would involve introduction of the "early return" pattern. This is historically a disputed pattern, meaning that some very much believe in it and others very much don't. In this case, for a book aimed at pretty early beginners, I don't want to get into that debate. As you said, it's a stylistic choice, which is why I'm encouraging readers to write whatever feels best to them. These solutions are described as one option, not the canonical best option.

Statement else seems like just redundant as for me

It's not... this range(..) utility does something different if you omit the second parameter (end) vs passing one in. If you omit (the === undefined case), it returns a function to be called later, which basically defers the calculation of the range until that later time. That inner function is where the closure is (remembering start), so it's pretty central to the point of this exercise.

But if you pass in anything (non-undefined, even null), the value passed for end is coerced to a number and the range is calculated and returned right away.

So these two clauses (if and else) are absolutely distinct.

sakastierov commented 4 years ago

@getify

That would involve introduction of the "early return" pattern. This is historically a disputed pattern, meaning that some very much believe in it and others very much don't. In this case, for a book aimed at pretty early beginners, I don't want to get into that debate. As you said, it's a stylistic choice, which is why I'm encouraging readers to write whatever feels best to them. These solutions are described as one option, not the canonical best option.

Got it and agree It's a matter of taste๐Ÿ‘Œ

It's not... this range(..) utility does something different if you omit the second parameter (end) vs passing one in. If you omit (the === undefined case), it returns a function to be called later, which basically defers the calculation of the range until that later time. That inner function is where the closure is (remembering start), so it's pretty central to the point of this exercise. But if you pass in anything (non-undefined, even null), the value passed for end is coerced to a number and the range is calculated and returned right away. So these two clauses (if and else) are absolutely distinct.

Thank you for such a detailed explanation The fact is that my remark was not about logic, but code style (again ๐Ÿ˜…)

Let's compare two implementations:

Current

function range(start,end) {
    start = Number(start) || 0;

    if (end === undefined) {
        return function getEnd(end) {
            return getRange(start,end);
        };
    }
    else {
        end = Number(end) || 0;
        return getRange(start,end);
    }
}

Suggested in PR

function range(start,end) {
    start = Number(start) || 0;

    if (end === undefined) {
        return function getEnd(end) {
            return getRange(start,end);
        };
    }    

    end = Number(end) || 0;
    return getRange(start,end);
}

So else { return (...) } And just return (...) Of course we cannot omit (...), but can omit redundant wrapper else { return (...) ~}~ in this case

Or am I missing something?

getify commented 4 years ago

Ahhh, so you're talking about early-return pattern in both suggestions. Yeah, so that's my response to both then. Thanks for the feedback.