Hacker News Reader

Comment by prepend | Hacker News Reader

It’s also really important to define defect during these reviews. I worked at a company where code reviews would identify lots of superficial errors like indention, improper documentation, and missing metadata. You want these in quality code based, but if this counts as a defect, it’s not as important as serious bugs being identified and corrected.

View on HN

If you search "Counting Defects" in the pdf posted by Morendil ( https://pdfs.semanticscholar.org/feb0/0dd349ad701e1e4045282b... ), you get some of the methodology on page 78 (the page with "78" written on it, not PDF numbering):

> When a reviewer or consensus of reviewers determines that code must be changed before it is acceptable, it is a “defect.” If the algorithm is wrong, it’s a defect. If the code is right but unintelligible due to poor documentation, it’s a defect. If the code is right but there’s a better way to do it, it’s a defect. A simple conversation is not a defect nor is a conversation where a reviewer believed he found a defect but later agreed that it wasn’t one. In any event a defect is an improvement to the code that would not have occurred without review.

I'd say that's a bit looser of a definition than I'd prefer, but it's at least good that the study didn't try to use some metric like "number of inline comments", since often times I use inline comments for praise or to mention possible alternatives that may or may not be any better than the code.

A real code review takes a lot of time. With the time available typically you can only find superficial things like style.

Not sure why a review would take so much time, mine typically take 20 minutes and I do two a day.

We ignore formatting (which can be formalized with a linter should we choose). Basically i check every line for reasonable error handling, review the whole for reasonable algorithm/data structures and i’m done.

What am i missing?

It probably depends on the work you are doing. We do test harnesses for hardware and most reviews are the result of a lot of experimentation and learning about protocols. In order to understand that code you need to know the characteristics of the device you are interfacing with. This may take weeks for a reviewer.

It depends a bit on context, I suppose, but if you spend most of your time/brainpower in code reviews on style issues, then I think you're probably not making a very good use of time. I think it's much more important to look at the problem being solved and the solution and to think through whether it makes sense. Often times I prefer to first have a conversation with the submitter where we talk through the structure and the decisions made, then I'll go through and leave inline comments for things like style as I see them.

I would put much more trust in a codebase built with careful engineering solutions than a codebase with careful formatting, and I think I would find it to be more pleasant to work with as well. Ideally you have both, of course.

I totally agree with you. However, in my experience the time you get for reviews is often not much and the developer already has moved on to something else and doesn't want to deal with the review. Obviously this is an organizational problem but one I have seen a lot.

On my team all code has to be reviewed before it goes to QA. We strive to do it same day, to make it more productive for reviewers and writers of the code.

It’s not too much overhead. Even though most reviews are satisfactory, occasionally it catches something that’s a good learning opportunity, and that’s golden.

Have you ever actually been constrained in how much time you can spend on code review?

I generally try to get my developers to spend more time on it than they think is necessary -- if it comes at the short-term pains of their productivity, I am totally ok with that and will work to revise their individual contributor expectations.

In most companies I have seen code reviews were scheduled in addition to the regular workload. This means you have to do find extra time to do them. There are also only a few people who can give a meaningful review of complex code.

I am now a believer in pair programming. Code review is already done during development time.

We just do code reviews on every commit, it’s just part of our flow. My experience is it’s far more productive than paired programming, our team doesn’t care about style issues and most code is clearly correct. So review of 4 hours of work can usually be done in less than 20 mins.

As far as cutting in the schedule, we get done what we get done. The schedulers can bite me, i wasn’t the one who mislead them to think dev estimates had any accuracy or usefulness.

My teams also assign code review "in addition" to the regular workload, but this is always in the context of an agile team effort: we get done what we get done [1]. A developer takes a story and works on it until it is solved (or splits it appropriately), then another developer (or a few) are expected to do review.

[1] I might be measuring productivity with story points in the background, but there is no "scheduled workload," this is merely an averaging/planning exercise.

The (correct, IMO) managerial effort to improve individual productivity is much softer and more understanding than that and comes with the understanding that your output takes many forms. Some of my best developers directly complete almost zero stories in a sprint because their time is spent on code review, pair programming, direct assistance and architecture discussion.

Reply on news.ycombinator.com