Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Code reviews aren’t just for catching bugs

blog.fullstory.com

Post reply on HN
Print view View original

121–130 of 150 posts

Re: Code reviews aren’t just for catching bugs

#121

Post by knorker »

wellpast wrote: post #11

What often goes unmentioned in praise for code review processes is their insanely exorbitant costs -- measured in engineer hours but perhaps more costly is all of the blocking and impedance [1]. Most of the "problems" that code reviews claim to address can be solved by much more direct and optimal measures. Code reviews are damn expensive. This post concedes that code reviews are better for the more fluffy ends -- te…

> insanely exorbitant costs

wat?

Either the code is easily readable and correct, and takes barely any time at all to review, or it's buggy and detecting this early rather than late saves a yuuuuge amount of time.

Also, the reviewer now knows the code. So in reviewing the code they've already half way to being able to improve the code in the future.

And the reviewer can also pick up "oh, that's a neat pattern". Code reviews are education for both parties. Are you saying companies should not spend time or money on education/courses?

And as an author I really appreciate not only that someone looks for me having made a mistake, but because I know someone will read the code I won't go with the easy way. I can fool myself, but I can't fool someone else. E.g. the right thing to do is to name this constant or make an enum, but I'll just put a literal "4" here because I know it's 4. (which of course I won't remember in a week).

Re: Code reviews aren’t just for catching bugs

#122

Post by asuffield »

voidr wrote: post #111

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 m…

(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.

Re: Code reviews aren’t just for catching bugs

#123

Post by qb45 »

neandrake wrote: post #95
maxxxxx wrote: post #62

How do people handle reviews of highly specialized stuff? We have people who do stuff nobody else on the team understands or at least it would take them a long time of learning to do a real review. I look at a lot of stuff and check if it makes halfways sense. I can look at the coding style but I can't judge the overall design without spending many hours on it (which I don't have. Nobody else on the team has it eithe…

Where I work the same situation often comes up -- a developer may spend weeks doing research for a specialized function building prototypes, etc. Ensure the specialized developer is doing due-diligence in defining and verifying the module works as intended and have others analyze its interactions in the larger system to prevent cascading failure, conforms to application norms, __is documented__, etc. Even if most oth…

> __is documented__

Documentation is indeed the name of the game. Few things are more frustrating than seeing a code do something and having no idea why.

I want to see comments, names of any clever algorithms used, links to applicable datasheets, research papers, etc. A short overview of the whole design is nice, too.

Re: Code reviews aren’t just for catching bugs

#124

Post by yitchelle »

thrownaway2424 wrote: post #92
msoad wrote: post #20

Code reviews can easily become a tool for people with huge egos to prove their smartness. I get code review comments for grammar of my comments or very small code style preferences that Google's anal style guide can't enforce (yet). I like code reviews, don't get me wrong. But there should be a way to respond with "you just shut up, you're only trying to make yourself look smart". It's all because higher up people mo…

I always complain about code comments that aren't complete sentences. Why should comments be misspelled or grammatically incorrect?

I also do the same thing. This is could sent he read down the rabbit hole if the comments is misleading which can be caused by misspelling or incorrect grammatical errors. Especially important if the team does not share the same language (ie English, German, French, or Mandarin etc).

Re: Code reviews aren’t just for catching bugs

#125

Post by oselhn »

voidr wrote: post #111

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 m…

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.

Re: Code reviews aren’t just for catching bugs

#126

Post by koja86 »

phasmantistes wrote: post #94
geophile wrote: post #80

Earlier quoted context omitted.

What? And keep the other developer waiting?

Code reviews should be asynchronous. Everyone on your team should be able to be working on multiple small changes/commits/whatever in parallel. While one is out for review, they're working on the others. Different people keep different schedules: I do all my reviews first thing in the morning, to settle in, and then often do another round after lunch. Other people do them at the end of the day, or don't mind the inte…

Context switching also creates some overhead. YMMV as the time it takes to switch tasks IMHO depends on particular person, context scale and problem difficulty.

Re: Code reviews aren’t just for catching bugs

#127

Post by zelos »

solipsism wrote: post #109
wellpast wrote: post #45

Earlier quoted context omitted.

Continuous integration means you can work on multiple items at once and that you break up large feature dev into smaller chunks that get committed to master. For example, I might commit the data layer for a new big feature well before the UX, etc. Code reviews mean that every time I want to integrate with master I've got to 'grab a lock' and schedule and wait for a code review. What typically happens in these code re…

I'm curious what you mean by grab a lock and schedule and wait for a code review . I've used all sorts of different SCMs, code review software, in-person code review processes, etc.. but I've never heard of anything like that. Could you describe your process to us in more detail?

I haven't done it myself, but I know of teams that use a lock for integrating changes to the main branch.

Without it the rate of change in the branch was fast enough that submissions would be preempted by someone else's and you'd have to rerun presubmission tests etc.

Re: Code reviews aren’t just for catching bugs

#128

Post by koja86 »

asuffield wrote: post #122
voidr wrote: post #111

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 m…

(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 wr…

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!

Re: Code reviews aren’t just for catching bugs

#129

Post by goda90 »

My employer develops safety critical software with decades of legacy code, long term support for multiple versions, and big customers. In our attempts to transition(in a mock way for the time being) to rolling releases we've "simplified" to a process with 3-5 people reviewing designs, 2-3 developers doing code review/light testing, 2-x(depending on affected modules) quality assurance people doing heavy testing and occasionally we pair program. Documentation/logistics is almost always more time consuming than design and development. I've had 4 character code changes take weeks to get through the process as several people have to find the time to look at it even for just an hour. It's cumbersome but we do catch a lot of bugs, and we're slowly getting better at design.

Re: Code reviews aren’t just for catching bugs

#130

Post by lmm »

fishnchips wrote: post #40
gpderetta wrote: post #16

Earlier quoted context omitted.

In my experience, no design document, no matter how carefully drafted survives contact with the enemy^W^W an IDE. At best you can define interface boundaries between independently developed modules. Edit: broken leftover line

Things change a lot during the project, I agree. But assuming everything will change and using that as an excuse not to plan at all sounds like a poor choice to me. "In preparing for battle I have always found that plans are useless, but planning is indispensable." - Dwight D. Eisenhower

> But assuming everything will change and using that as an excuse not to plan at all sounds like a poor choice to me.

I know that's how it sounds. But in my experience it turns out to be a lot more effective than planning.

Post reply on HN