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

131–140 of 150 posts

Re: Code reviews aren’t just for catching bugs

#131

Post by waigani »

This is a topic close to my heart. I’m a software engineer for Canonical (company behind Ubuntu). Last year I did a study of the total review wait time over a two month period, which came out to be 8683hrs ( http://lingo.reviews/d3/juju_review_wait_times.html).

I wrote up my thoughts around scalable engineering here: bit.ly/1P0YgNo and released an MVP solution here: www.lingo.reviews

Since then I’ve been refining privately with a handful of engineers from different companies. Two days ago I put in my notice and took on solving the problem of scalable engineering as a full-time mission: http://codelingo.io

I'd love to connect with anyone that is passionate about this problem (it's been a 2 year obsession for me): jesse@codelingo.io

Re: Code reviews aren’t just for catching bugs

#132

Post by kerkeslager »

r-s wrote: post #101
kerkeslager wrote: post #70

Earlier quoted context omitted.

> 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. I'm on a team where for about a year, part of our process has been that code…

I have a hard time imagining that most code reviews take * Understanding of the problem at hand, reading through the story, understanding the context of the the code change. * Pulling down the code, reading through the commit log * Reading the specs, possibly running coverage tools if not part of testing suite. * Understanding the logic of the code, seeing if the tests cover the edge cases * If dependencies change, t…

* We mostly understand the problem at hand already because we were in the same planning meeting.

* We do our code reviews on GitHub by looking at the diffs. No need to pull code.

* We review the code of the tests, but we don't run them. Our CI server runs tests and coverage tools.

* The code is usually a short MVP, * If dependencies change, it was a group discussion and we already investigated.

* Again, small commits make this quick.

* Small commits make this quick.

* Documentation is handled by our product manager.

I guess what I'm learning from reading your comment is that part of why our code reviews are fast compared to yours is that we mean drastically different things when we say "code review".

Re: Code reviews aren’t just for catching bugs

#133

Post by eterm »

Domenic_S wrote: post #50
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…

There's a line to walk for sure. When I code review a line that looks like this: $foo=fn1($bar) &&$baz = fn2 ($qux) I always send it back. Do I look pedantic? Probably, but to me consistency is important. It's about caring about what you're doing. If I see a line like this, I assume the person who wrote it doesn't pay a lot of attention to detail and I pay closer attention to the rest of their commit. I mean, if you…

Isn't that in part a linting issue?

I figure code reviews are for reviewing stuff that can't be corrected by a computer.

Re: Code reviews aren’t just for catching bugs

#134

Post by jclulow »

koja86 wrote: post #128
asuffield wrote: post #122

Earlier quoted context omitted.

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

Site Reliability Engineer?

Re: Code reviews aren’t just for catching bugs

#135

Post by voidr »

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…

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

Re: Code reviews aren’t just for catching bugs

#136

Post by voidr »

oselhn wrote: post #125
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.

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.

Re: Code reviews aren’t just for catching bugs

#137

Post by voidr »

aavz wrote: post #115
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…

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…

> 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?

Re: Code reviews aren’t just for catching bugs

#138

Post by Cthulhu_ »

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…

Taking the time and effort to comment on grammar or code style preferences is anything but attempting to prove smartness; it's stupid work that shouldn't be necessary if the one submitting the code for review paid more attention to those basics.

If you can't write a coherent sentence or consider a style guide 'anal', you have no place developing for a project that is serious enough to warrant code reviews.

Another thing, if someone submits a PR with basic errors like that, they distract from the things that should be looked for in a code review - bugs, implementation details, etc.

Re: Code reviews aren’t just for catching bugs

#139

Post by Cthulhu_ »

BurningFrog wrote: post #24

Maybe I'm an arrogant XP-ist, but to me this sounds like a good step on the way to pair programming.

It is, but with some differences; works better for remote teams (see github), is async (don't need to interrupt someone to get cracking), is more suitable to introverts (or anyone really, who can pair program for 8 hours / day or more?), and you get to sit down and review someone's code with a fresh pair of eyes instead of not catching mistakes because you wrote them or were there while they were being written.

Re: Code reviews aren’t just for catching bugs

#140

Post by wandernotlost »

db1 wrote: post #104
wandernotlost wrote: post #87

While I value many of the same things as the author, I've found code reviews to be far inferior in every respect to pairing (especially promiscuous pairing (google it)), and to have negative effects in several important ways: * They delay integration. * They tend to encourage focus on abstract code polishing without proportionality or relation to the value of the code. * They favor superficial improvements, while inc…

Interesting, this is the first time I'm hearing about promiscuous pairing. How big the team at your company is and what percentage of time is spent pair programming?

I do consulting, so I work with a lot of different teams of various sizes and configurations. The best teams tend to pair close to 100% of the time and generally have 4-10 developers. Fewer than that makes it difficult to swap around, mix perspective, and keep things fresh, and more than that starts to introduce more complications in planning and coordinating work streams and sharing context.
Post reply on HN