Hacker News new | ask | show | jobs
by voidr 3693 days ago
Code reviews:

* signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted

* can create a culture of passive aggressiveness - you do something that I don't like, I'll get back at you when I'll review your code

* not guaranty code quality - having a junior review a junior's code will not yield expert level code

Just because Google does code reviews doesn't mean you should, for Google a bug could cost millions, for your project a bug might cost 10$, however the overhead of code reviews might cost more than 100$. It's important to do numbers and think rationally.

5 comments

(Tedious disclaimer: my opinion only, not speaking for anybody else. I'm an SRE at Google.)

I distrust me by default, and you should too. Humans cannot write correct code, and the way to keep high code quality is to maximise our ability to fix the errors that result from having humans involved. I don't want me submitting any code that hasn't been looked at by another person. While I do (almost every day) manage to write some CLs that get approved without comments, I definitely have many CLs every day where somebody will say "This is confusing" or "There's no test for this part" or "Here's a better idea that I had".

If your team members are engaging in passive-aggressive abuse then you should find new ones, not try to do your job without interacting with them.

A person of the same experience level as me will routinely find things that I missed, just because they didn't spend two hours writing the code and are taking a fresh look at it. The same thing is true of a person more junior than me, if we can make them not be shy and write comments like "I don't understand what this does, therefore it is too confusing". No reviewer guarantees code quality, because nothing guarantees code quality, but my experience is consistently that 1 reviewer is a massive improvement over 0 reviewers, with marginal improvement based on reviewer experience.

The true cost of not doing code review is that your code will be harder to maintain in future, giving continual costs for its entire lifespan. The only code I consider to have a cost/benefit ratio that makes it worth skipping the review phase is code that I don't intend to keep for very long.

> Humans cannot write correct code, and the way to keep high code quality is to maximise our ability to fix the errors that result from having humans involved.

This kind of breeds a dilemma: If humans don't know how to write correct code, why are we trusting them to verify code correctness? :)

No matter how many times I run jslint, I always get the same result, however if I would show the same code to 20 programmers, I'm pretty sure I would get a lot of different results.

> If your team members are engaging in passive-aggressive abuse then you should find new ones, not try to do your job without interacting with them.

If your friend looks at your code, (s)he'll find a lot less issues than your rival. We software engineers are not emotionless objective beings.

My issue is that I have seen few issues that could have been caught in time by code reviews with the cost of the code review being less than the cost of just fixing the problem. Code reviewing every change is continuos effort, that might cost more than having a few quirks and fixing it.

I do understand some projects do require every kind of verification process you can throw at it, like software that controls nuclear power plants, however not everyone is building that kind of software.

Regardless of what SRE actually means I guess it must start with Senior. I have yet to hear official definition of that word but self-reflection, realization of own limited abilities and means-to-an-end mentality are something I personally would consider a strong candidate. Awesome mindset sir!
Site Reliability Engineer?
Assuming that people will make mistakes writing code isn't distrust, it's just common sense.

If someone is a good developer, I trust their first cut of code will be well thought out. I don't assume that they considered all corner cases or found the best design or written things in a way that makes sense to other people on the first try.

Things go downhill if people start taking code reviews personally, but then I don't think the real problem is code reviews.

> I don't assume that they considered all corner cases or found the best design or written things in a way that makes sense to other people on the first try.

Are you then assuming that the person who will review the code will consider everything?

signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted

That doesn't have to be a bad thing. You (and everyone else) can't be trusted to be totally infallible, so if you want to produce good work relying on many eyes to catch mistakes, suggest improvements, or to learn from one another then you need to look at everyone's code. A code review should be a conducted in a safe, blame-free environment where everyone involved wants to make better software. That's should be the goal, not finger pointing or points scoring.

It's not just programming. Journalists and authors have editors for this exact reason.
My boss performs code reviews before my work is delivered. In my case it is mostly in a positive way: - he wants to know how stuff is implemented because he may need to maintain it if I am on leave. - it is a motivation for me to produce good code because I know it will be read - he may give good advice (for example usage of ArrayList instead of Vector in java)
This is not code review problem but huge ego problem. When you are discussing code you should always use technical arguments not personal attacks. Only bad programmers value their ego more than good advice from others. You should always use code review as opportunity to learn/teach new things not to fight or offend others.
I have yet to work in a team where everyone is 100% on the same page, agree on absolutely everything and totally objective and emotion free.

The reason I don't trust these methods is because they seem to ignore human nature.