Hacker News Reader

Comment by Maro | Hacker News Reader

One way to keep this from happening is code reviews. Don't show me a presentation, don't show me blocks of architecture, let's open Github and start looking at the code. Nothing formal, just a 10 minute walkthrough and discussion. Unfortunately in many Eng cultures the EMs stop doing this, or do it too late. In my mind this simple idea is similar to the Toyota Production System's "Go and See for Yourself (Genchi Genbutsu)".

View on HN

people feel bad about publishing a PR and being told "don't do it like this" and get defensive, argue about the sunk cost, throw references justifying why their approach is Good Actually, accuse the reviewer of not being serious about software engineering, etc.

The real way to project success is egoless programming.

started on this project a few months back and have had PRs blocked with stuff like

"in this test file, just use one assertion, don't need to use two"

"there's too many comments here"

"i don't like the extra line breaks in this file"

"bookAdminId needs to be bookAdministratorId"

these are generally comments just on test files.

I've struggled to deal with these because... well... ego is part of it, but when there are bigger structural issues, and the focus is on - imo - minutia (without much acknowledgement that these are, indeed, minor points), it's troubling. To then be told after a few weeks that you're "not delivering" is doubly so. It's hard to take 'ego' out of this altogether, but thanks for reminder.

Yeah this sucks. the egoless approach is to suck it up and do the changes, but also to raise with the team the difference between feedback for implementing the feature & expressing opinions. One thing I liked about Review Board (!) was making comments as 'Issue' (to make it clear this had to be fixed before merging) vs. not (making it just opinion/non-critical feedback). Though in a team of pedants you would find every comment being marked as an issue

what's "review board"?

also, it's triply bothersome because... i've been doing this > 20 years. that certainly doesn't mean I don't make mistakes, and am never wrong, but it does get tiresome to have whitespace arguments take up time 2 decades in. On top of this, there's actually an automated linter that we all adhere to, but these are 'extra' style things that someone likes to focus on outside of the linter. It's just wasteful of the company's time, more than anything else.

Review Board is a Django-based code review tool, like if you had a separate service for merging PRs in Github? It does a lot of things wrong (e.g. doesn't keep track of commits in a change request, flattens all the changes) and 2 or 3 things right (fix-n-ship, multi-line comment)

Stuff like this is why I've become a huge advocate for automatic formatting tools. If you can set rules for naming conventions, line breaks, etc. then it significantly cuts down on the minutia arguments.

It doesn't fix everything, of course. Someone can still argue about using too many assertions or comments but it at least helps. If someone makes an argument about formatting then you can just say "well, Tool X formatted it this way so this is how it will be. If we think that should change in the future then let's open a different issue and discuss that."

Code formatting tools/linters and static analysis tools can deal with the bulk of this before it even gets to review.

For the small stuff that remains, ideally the reviewer can fix it - the reviewee is probably back in flow on another task, and it's irritating to have focus broken for inconsequential things.

As a side note, I like how github implemented this as "review suggestions", where the reviewer can make small changes inline and the reviewee approves them with a button push.

Man, I feel for you. You could be working with an ex-colleague of mine, from all you've said. I inherited _his_ codebase when he was let go, and had to re-write most of it, there was no way to fix that in a reasonable amount of time, if even at all.

But the same type of things on the PR comments, the over-architected, highly-coupled monstrosity that never quite worked.

In the end, we ended up with 6K fewer lines of code (out of a total of 16K or so), and the thing actually works now. Go figure...

IIRC, code reviews were done, just... no one really saw the entire 'big picture' and how unwieldy this was until the very end.

Reply on news.ycombinator.com