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

31–40 of 150 posts

Re: Code reviews aren’t just for catching bugs

#31

Post by mikestew »

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…

And those same ones commit stuff like the LoC I'm looking at right now that returns the results of a ternary, and there's a fucking bang symbol in front of every boolean, topped off with a bang preceding the parens around that ternary (IOW, some of those bangs should cancel out, and aren't needed). I ask myself, "how did this escape code review?" and the answer is probably that the senior dev who wrote it thumped the junior dev reviewing it with something along the lines of "that's how real Java devs do it", or something. I dunno, just guessing based on personalities. Same dev that self-admits he hasn't written unit tests before, yet argues with me (who has written thousands) about how to write unit tests.

Though I wonder if what you describe isn't just plain ol' bike shedding. I've seen it plenty in code reviews. Super sharp dev who generally writes great code puts a commit up for review, and someone feels like they ought to have some input, and because the code is otherwise solid they pick on grammar.

Re: Code reviews aren’t just for catching bugs

#32

Post by ben_jones »

buro9 wrote: post #2

I'd still rather earlier code reviews... design reviews about 1/3 of the way into writing the code. Enough time to have passed to have discovered the dragons in the whiteboard design, but not enough to have written code that could only undergo minor fixes in a code review. I prefer the idea of a code review happening at a time that it could still steer the ship... too many code reviews catch bugs, but don't correct p…

I think it will always depend on the team. Process X might be a savior to team Y, but completely screw team A even if team A == team Y (javascript truthiness here).

On the flip side I think you can also say the potential downsides of excessive code review are significantly less then the potential downsides of zero code review.

I dunno.

Re: Code reviews aren’t just for catching bugs

#33

Post by wellpast »

chris_va wrote: post #17
wellpast wrote: post #13

Earlier quoted context omitted.

It's not arrogance. You don't have to write perfect code to realize that the insane cost of code review is not worth the problems it is supposed to solve. Especially when code reviews are notoriously not very good anyway at excising bugs. The goal isn't perfect code. It's optimal delivery of business value. Code reviews are expensive, and not very optimal.

Why do you find them expensive? I've done thousands in my career. They don't take much time compared to actually writing the code, and adding an extra 5% of engineering time pays major dividends later without drastically reducing throughput.

For me continuous integration and refactoring have been the most important practices in keeping a code base clean, robust and agile.

Code review cultures tend to encourage the opposite of these agile practices: monolithic commits, infrequent integration, and minimal diffs -- in other words, practices that tend to result in lesser productivity.

Re: Code reviews aren’t just for catching bugs

#34

Post by UK-AL »

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…

The easiest is just fix it, even though its annoying.

Slightly clearer comments isn't a bad thing? Why not fix it in the two seconds it takes to.

No one's going to repremanded or promoted on code reviews.

Im a senior dev(ops) and don't mind if I get code review full of really minor issues by junior devs try to prove themselves. Every little issue fixed, makes a it a little bit better.

Re: Code reviews aren’t just for catching bugs

#35

Post by CorvusCrypto »

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…

I feel like this can be argued so many ways. For instance, while code review might block some changes from being introduced into the system, it also does have more benefits than the "fluffy" ones you mentioned. Maybe an engineer that has more experience with the codebase can point out a way of doing something that is more in line with what the team's style is (earlier move toward standardized code vs letting people do what they want). Maybe you missed a utility function that already achieves what some of your code does. Maybe there's not a bug, but again a more experienced dev sees something that can be optimized and will save time in the future that would be spent on tracing system performance issues. The idea that we need to move ultra fast as devs... I think that is necessary in some cases, but I'm not convinced that taking some time to code review tasks is as expensive as you suggested. I mean even reviewing 1000 lines of code should take no more than 30 minutes. And if it looks entirely confusing and hard to follow, then maybe it's something to revisit because I guarantee you're gonna be spending longer if you have to go back and refactor/extend messy and unclean code that technically works. I think the key here is to catch things that stand out and not review the minutae.

This is just my opinion of the matter. On the topic of team building, I don't really care for all that stuff. Unit tests w/ CI and periodic refactoring are great for revisiting design decisions. Code reviews are great for catching bad design decisions early on, though so use it as a tool.

Re: Code reviews aren’t just for catching bugs

#36

Post by mikestew »

hangonhn wrote: post #7

I would go further and say that you should never trust code reviews to catch bugs. They're great for all the reasons in the article and what "zhemao" said but they're horrible for catching any bugs beyond the most trivial ones. Humans simply aren't good at running code in their head like that. Code review is no substitute for good code coverage and automated testing, preferably pre-commit.

That was going to be my comment on the title. If your code reviews are "...just for catching bugs", you're doing it wrong. I put another comment in this thread describing a crappy line of code. It works (I wrote the test for it), but my...$DEITY could it have been easier to read, and made more maintainable. But it apparently sailed right through code review.

Like you, I'd say that code review isn't for catching bugs at all. Is it readable? Can some Shmoe off the street who is hired to maintain make sense of it? How's the complexity? Do I need to maintain eight different states in my head to grok it? Et. al. Now, a team can help prevent potential bugs (either outright as the code stands, or bugs introduced later when someone tries to maintain it and it's a pasta derivative) with code reviews. I mean, didn't we run the unit tests we wrote before we submitted it for code review? Why are we finding bugs just by looking at the code?

Re: Code reviews aren’t just for catching bugs

#37

Post by eduren »

mikestew wrote: post #21
eduren wrote: post #12

Working at a Gov't IT contractor, I really wish we had the organizational skills/incentive to do code reviews. Very rarely does the code I write get glanced at, much less examined. My instinct, of course, is to solicit code reviews from my peers, but the organizational structure and support tooling are all woefully inadequate. Plenty of projects, even greenfield ones, aren't checked into source control. And of course…

Plenty of projects, even greenfield ones, aren't checked into source control. I really and truly did not know this still happens. Hell, even on throwaway/PoC stuff for which I am the sole developer, and code that stands a good chance of never seeing the light of day, I start with git init . 'cuz the probability that I'm going to wish later that it was in source control outweighs the very minor cost of putting it in t…

I agree. Source control is an integral part of my workflow and it hurts to know that if someone here were to maintain my code later, they'd just copy and paste it as-is and start from there. The reasons for this are two-fold:

1) Like you said: Inertia. Most projects/developers here have been around for years, many starting before git was a thing. SVN is around and used quite a bit, but like I said, I've talked to developers that use neither. Since the code lives on the server, and the server is backed up, people feel no need for source control. Which is part of...

2) At least in the web development I'm doing, there's little to no "collaboration". I have been the sole person actually writing code on every project so far. There are teams working together, but usually every project has a single developer. This place is so vulnerable to their developers getting hit by a bus. But management doesn't care because 12 months down the line, once the developer is no longer working the project for whatever reason, the project is scrapped and either re-done because no one else was involved in the technical details, or they spend another year in federal procurement hell to get a shitty off-the-shelf product.

Re: Code reviews aren’t just for catching bugs

#38

Post by adrianN »

eduren wrote: post #12

Working at a Gov't IT contractor, I really wish we had the organizational skills/incentive to do code reviews. Very rarely does the code I write get glanced at, much less examined. My instinct, of course, is to solicit code reviews from my peers, but the organizational structure and support tooling are all woefully inadequate. Plenty of projects, even greenfield ones, aren't checked into source control. And of course…

Maybe you could be your own reviewer. Just pick something you did a month or two ago and check whether it still makes sense.

Re: Code reviews aren’t just for catching bugs

#39

Post by renku »

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…

What are these "more direct and optimal measures" that are cheaper than code reviews?

In my experience code reviews are much cheaper than other means of raising software quality. For example unit tests only start to add real value after you have written a good bunch of them, so they form this regression-safety-net.

I haven't seen Continuous integration and refactoring being thrown out of the window because of code reviews. Quite the opposite. Code reviews usually ask the author to refactor his code further. Similarly I have no idea why one would stop continuous integration because of code reviews.

Re: Code reviews aren’t just for catching bugs

#40

Post by fishnchips »

gpderetta wrote: post #16
fishnchips wrote: post #5

Earlier quoted context omitted.

This is the role of design documents which get heavily reviewed by the team before even the first line of code is written.

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

Post reply on HN