Hacker News new | ask | show | jobs
by emerongi 639 days ago
Another part of a good code review is to pick your battles. I used to comment on every little thing. Now I have a threshold below which I won't bother unless it would significantly improve the readability or runtime characteristics. Your coworkers' sanity is just as important as the code to ensure that the team achieves their goals.

If I feel like the author might just not be aware of some "better" (subjectively) way of doing things, then I'll leave a "FYI: could also do this in X way" type of comment.
2 comments
gnutrino 639 days ago
The “suggest change” feature in github is a great way to suggest nitpick fixes without coming off like a jerk. You actually do the work, and the author can easily merge in all those changes with one click. They’ll avoid making those small mistakes / style choices over time. Also it feels more collaborative and less “do this because i said so”.
link
Fire-Dragon-DoL 639 days ago
It's bugged and doesn't allow more than 6 to be batched, but yes
link
mgkimsal 639 days ago
My threshold includes time/delay. Knowing that asking for some fix is going to delay this thing getting done by a long time - sometimes days - impacted my caring about certain things. Sometimes it was easier to just make a small fix myself than 'request' it and wait for 3 days just to get something fixed.

Personally, I would prefer someone actually make the suggested fix they're describing - doing the actual code - so I could see it fleshed out. Commenting "use a generatorInterface" to someone who's probably not used it before is less helpful than actually implementing the generatorInterface in the PR and then discussing the actual code. Not always time for that, but if it's never considered, there's never any time for that approach.

tanget: I had a PR blocked because... "use more descriptive variable name" was applied to a 'for (i=0; i<upperBound; i++) ...' loop. The complaint was about using 'i' in the for loop. This was in a test file - the first test file on the project that had been live for 7 months - and I think this nitpick was just a bit over the top. And it wasn't enforced later, just ... someone making a stink over 'a new guy' joining and trying to exert some influence.

link