Hacker News Reader

Comment by pimlottc | Hacker News Reader

Even with a code review, though, you might want to wait until you’ve digested the rest of the review instead of firing off a comment. Maybe it makes sense in context. Or maybe there are bigger fish to fry and it’s not worth quibbling over small things.

View on HN

> Or maybe there are bigger fish to fry and it’s not worth quibbling over small things.

I've taken this approach with one of our engineers, and now we have a huge pile of "small things" that has created some pretty serious technical debt.

My perception is that they don't try to understand what I'm pointing out, and come up with rational for how they have it. I think I have a lot to learn.

I'm still learning too, but I think that something I'm trying to develop is a sense for the longer-term costs and implications of apparent defects.

- Is this a component that every engineer is going to interact with and that will stay with the company for decades?

- Can this technical decision be reversed easily? Either in the sense of an individual commit revert, and/or in terms of a series of code changes?

- Is this change likely to remain isolated to this part of the codebase, or will the pattern infect or be copied to other locations widely?

It's super challenging -- and also intellectually rewarding when you can master it and gather team consensus and alignment.

> Can this technical decision be reversed easily?

One of the most exhausting failure modes that I've had to deal with is deciding not to decide. Some people will build elaborate Rube Goldberg machines so that they "can change their minds later" but in fact what they've done is decided that choices are now everybody else's problems.

There's a whole hell of a lot of rewards to having the maturity to be able to say "I was wrong" and move on. Writing code in a way that you could rework it to use a different library to accomplish the same thing is making a reversible decision. Creating a rules engine to let a config file make the choices and leaving a cartesian product of states (75% of which are invariably illegal, which you are just supposed to know) is definitely, definitely not. Just pick something, man.

I definitely have a list of things I've let go just to be nice that have caused production outages, consistent poor performance, time-consuming technical debt, etc. It's great to have a good relationship with your team, but at the end of the day, the things that you "let go" can become reasons not to buy your product or why new engineers won't be productive on the codebase. It's very easy to be the person that never says "no" to something, but at the same time, it's also easy to be the reviewer that doesn't advance the reviewee's career. A reviewer should ask themselves; "by not commenting on this, am I making the reviewee a better programmer?"

Something I've noticed is that acquaintances will be nice to you universally, but actual friends know when to broach subjects that may not be considered "pleasant" or "nice". Don't give your friends "acquaintance-level" code reviews.

I have found personally only pair programming with people regularly is the only way you can avoid adding lots of technical debt.

Technical debt isn't always avoidable, but I sure know the difference from when teams are heavily invested in pair programming vs not.

Good pair programming is hard to ingrain in a culture though.

It is tough to balance between sweating the details and not bikeshedding (sweating the details at the expense of the big important architectural decisions)

I think it's a matter of picking a few things you're going to comment on, and any time the person actually stops needing to be corrected on one of them, you add one of the others in. Any good teacher or mentor does this. But it's a balance because you need the student to see that the reason you're complaining about something new is because they have in fact progressed, and we have moved on to the next thing to address (which has probably always been there)

This is why I am so grateful for GitHub reviews; Being able to group comments to fire off all at once has saved me from this multiple times as I continue reading, realise something, and remove a previous comment.

In fact you reminded me of the technique right now to check the responses and make sure nobody else had already said this!

On the other hand, someone coming after you might read the code in the same order and have the same wtf moment. They may even never find that later piece you saw in the review. So at the least it should still be a signal to the Author that it appears strange.

I also really like this feature of GitHub reviews and when deleting a comment as I continue reading I try to question whether to add a code comment so the next person doesn't get tripped up because they don't have the full context in their head.

> Or maybe there are bigger fish to fry and it’s not worth quibbling over small things.

This was my excuse to not be thorough when giving feedback in code review, when I was last in a position to do so, while on the Windows accessibility team at Microsoft. I told myself I was on a mission to make Windows more accessible, and there was no time to delay new features or fixes over inconsequential things like coding style or even code organization, as long as it worked.

Edit: At least I didn't use that excuse when responding to coworkers' reviews of my PRs.

This is a tooling problem. Github reviews vs. Bitbucket reviews will show this. In Github you can write your comments and send them all at once. Until you submit the review, you can just edit your comments and undo them or whatever.

Reply on news.ycombinator.com