I wrote the code that Laurie reviewed, and I do agree with everyone criticising it. The spreads in particular are very inefficient. In my defense, perhaps I should explain what the code was actually doing. The real obj1 and obj2 were actually packageJson.devDependencies and packageJson.dependencies, and originally I had been creating a combined list of dependencies, which I was then using elsewhere. When I simplified it into a single check, I didn't then also refactor out the spread. But that's what code reviews are for! The version that I ended up committing was something like:
Thanks for the answer, nice to know the "behind the scenes" 😄
I think in another comment Sebastian mentioned that depending on the target audience of the code, it may or may not be relevant to use some code conventions.
Personally when I see "!!" I know what it means but the first time I encountered it I was really puzzled 🙂
But again, context is everything. In this case, I'd say that the fact that the function is named "isGatsbySite" is more than enough to clarify the code.
Software dev at Netflix | DC techie | Conference speaker | egghead Instructor | TC39 Educators Committee | Girls Who Code Facilitator | Board game geek | @laurieontech on twitter
3 lines of clear code > 1 line of cryptic code
😛
Oh, that's a keeper. :D
I wrote the code that Laurie reviewed, and I do agree with everyone criticising it. The spreads in particular are very inefficient. In my defense, perhaps I should explain what the code was actually doing. The real
obj1andobj2were actuallypackageJson.devDependenciesandpackageJson.dependencies, and originally I had been creating a combined list of dependencies, which I was then using elsewhere. When I simplified it into a single check, I didn't then also refactor out the spread. But that's what code reviews are for! The version that I ended up committing was something like:...except it was actually TypeScript. The
!!was because the return value was a boolean. I supposed I could've used aBooleancast there. 🤷♂️Thanks for the answer, nice to know the "behind the scenes" 😄
I think in another comment Sebastian mentioned that depending on the target audience of the code, it may or may not be relevant to use some code conventions.
Personally when I see "!!" I know what it means but the first time I encountered it I was really puzzled 🙂
But again, context is everything. In this case, I'd say that the fact that the function is named "isGatsbySite" is more than enough to clarify the code.
I’d argue that it was a reasonable way of doing it. Was there a way to simplify it? Sure. But it wasn’t egregious. Plus, I learned something :)