Comment by bpyne | Hacker News Reader

Comment by bpyne | Hacker News Reader

So many good lessons for newer developers and good reminders for more experienced ones. It was due to the Bike Shedding Effect that I came to not appreciate code reviews. Every review seemed to break down into discussions of variable and method names. I tried several times to create "off topic" rules like no discussion of variable names, etc. It didn't work...ever.

Now that I've been away from mandated code reviews for quite a while I can understand why reviewers go to the trivial. Firstly, "code reviews" are really "system reviews". It's difficult reviewing a project whose requirements you don't know with a design that came about through iterative design and possibly in a language you don't use.

My current team is small and split into an integration group (mine) of two people and a UX group of two people. My group partner and I look over each other's projects and stay up on the requirements as the project progresses. But it's completely informal. Yet it seems to work better than having a group of 2, 3, X number of randomly chosen developers from my department.

That’s why pair programming works better than code reviews. Unless you have crazy amounts of time there is almost no chance a code reviewer can really understand the design of the change. So you end up with people picking on very local stuff like variable names. With pair programming you have two people who understand the history of the code and why decisions have been made.

But seriously, naming things correctly makes understanding the global system so much easier.

The whole point of naming things is that you don't have to dive in this 60 LoC function, but rather can use its name to know what it does, and make a global mental model.

If you're not naming things correctly, you effectively make it almost impossible to review the design, and that's why people starts with asking about names.

Reviewing names is the exact opposite of picking local stuff. Names have no importance locally, but they have tremendous one globally.

So, please, use good names ^^

Totally agree about the importance of naming. But the big problems that are hard to fix later come usually from design flaws, not from naming and often are overlooked in reviews.

I agree with you. My point, if it wans't clrear, is that I think correct naming is a prerequisite to finding design flaws in review ! Hence we have the same ultimate goal :-)

I don't find that to be the case. I'm usually looking for bigger things like: 1. making multiple trips to a relational database when a single statement joining tables can make it one trip; 2. not handling exceptions that can cause data loss or data corruption.

(I'm not trying to be exhaustive. The examples above are common ones I've seen regularly and they make me cringe.)

I don't need to know the variable names used in these cases. The very act of not using a relational database properly or not handling exceptions is easy to spot.

That doesn’t sound right. How big are the changes you’re reviewing?

Sometimes small and easy, sometimes big and impacting the whole system. Generally small changes are preferred but that’s not always possible.

We generally review anywhere from 50 to 300 lines per diff. An unfamiliar reader may not be able to anticipate spooky action at a distance in other parts of the codebase, but a good design doesn’t permit much of that anyway. The reader can certainly understand what’s going on in the change, and if they don’t they shouldn’t approve it.

Some of our changes are bigger if they touch several systems. You have to have very good knowledge of each system to make an assessment of the impact of the change. And nobody has time to really look into this. You don’t get much credit for code review on the scrum board.

I think code review is still valuable, but I agree that the majority of actual reviews are surface level and unhelpful. On my team right now there are like three different people who all have different opinions on how code should be written, and all bikeshed it differently. It's really interrupted my flow lately, because I'm thinking less about what to write and more how I can do it in a way that will appease the reviewers. And then I get differing feedback anyway and have to spend my time mediating between the reviewers who don't even agree themselves.

Reply on news.ycombinator.com