A harsh critic.

Today I did a bad thing, I let go of all restraint during a code review and pointed out all the inconsistencies within a file that my boss wrote... I know, I know what was I thinking!

A review in my eyes, it's not for you, the author, it's for the next person to work on this same code. And sure the code works, but smaller details should not be ignored... Maybe, the truth is I usually bite my tongue under review sinse anything I feel is valuable is just an opinion. My opinion is of fair usage where anything that should be there is there and nothing extra is needed.

Breaking my rule was empowering but unfortunately got me kicked off of the review, that's never happened before and was a complete shock to me. I suppose my job in review isn't to be a human linter.

With a heavy heart I just want to crawl under a rock for the humiliation. But at the same time if my boss had just spoke to me first, I'm sure I would have improved my comments to be more constructive.

Comments

[Ben Lovy]

So...as an industry outsider, what you describe is quite literally what I had assumed a code review was supposed to entail. Seems like even if what you offer is an opinion, perhaps it's valuable and can be then assessed objectively against everyone else's.

Why was this such a faux pas? Are you saying you think it was a tone/communication issue, and not the substance, or that your substantive comments were actually unwelcome regardless of merit?

I guess I'm curious what your actionable takeaway for the next similar situation is.

[Adam Crockett 🌀]

I think it was tone. I was on a high, having been approved for some work, I flew through the review feeling pretty good and not really thinking.

Ill give an example.

const thing = {
    someMethod: () => {}
}

thing.method2 = function (){}

So the above code works, but I'd rather see either the dotnotation assignment or the whole object declared, not both. I'd also consider using shorthand method declaration over arrow functions if the scope of this is not required.

I think that's all valid?

But I had so much to say about this one file. It's more that I wanted to show that I am paying attention. Iv been going through a rougth patch at the moment and all I want to do is get my confidence back.

[Octa]

As a full on beginner, could you point to resources that explain the reason behind your comments? I tend to generally go with arrow functions a lot.

I can't comment on the technicalities (obviously, from the comment above), but it seems like the human side of the entire affair could have been handled better, or at least with something else than kicking you off the review. I hope you regain confidence soon.

[Ben Lovy]

I'm not Adam, but this post gives a thorough discussion of why it's not a great idea to use arrow functions specifically as method definitions, under the heading "Where You Should Not Use Arrow Functions" a little over halfway through. They're fine to keep using in general, just with caveats.

The short answer is "JavaScript", the medium answer is that arrow functions are like closures or lambdas if you know what those are from other languages, which means they operate in their parent's calling context as opposed to creating their own separate context. This can cause unintended behavior at worst, or at least lead to code reliant on implicit as opposed to explicit behavior which hurts readability.

Also a beginner, though, Adam may have a better/more correct take.

[Adam Crockett 🌀]

Yep pretty much my opinion on arrows for example. They don't make sense everywhere, even if they do work everywhere. And your sighted post is bang on the money ... Imo 😁.

[Ben Lovy]

Gotcha - it sounds like more a human than a technical problem to me. Your example criticism here seems both uncontroversial and correct.

I guess from an emotional standpoint I could see "shorthand method declaration over arrow functions" construed as a nitpick as opposed to an engineering note, especially if it's a lot of like notes at once, but that doesn't make it not fundamentally correct, and you may disagree - it's actually more important than that. In that case, you'd hope the next step is a discussion of why or why not to make the change, not getting kicked out of a review.

Sorry you had this frustrating experience. FWIW, my limited perception is what you had to say about this file wasn't actually out of line, and it's discouraging that your effort to demonstrate a high-quality review was met with such a disconnect. Again, I really thought this was all exactly the point of reviewing code.