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

How I review code

engineering.tumblr.com

Post reply on HN
Print view View original

11–20 of 144 posts

Re: How I review code

#11

Post by watwut »

gizmo686 wrote: post #10
watwut wrote: post #9

Earlier quoted context omitted.

It continues with "and need to be explained why each change is okay". If reviewer suspects there is a bug, reviewer should check for it instead of having reviewee explain him or defend every detail. It is micromanagement this way - not just being similar or kinda like micromanagement, but it is literally it. If every five lines big code change in run of the mill fronted requires two hours long negotiation, then somet…

>If reviewer suspects there is a bug, reviewer should check for it instead of having reviewee explain him or defend every detail. It is not a matter of if the reviewer "suspects there is a bug", but rather a matter of if the reviewer is convinced that there is not a bug. If the reviewer needs to have someone explain why every minor code change is correct, they are either lazy or incompetent (or the code is badly writ…

And other times, reviewer is doing that to look detail oriented and responsible. He can do that so everybody sees how great and attentive he is. It is opposite of laziness in such case, but still unfair to reviewee.

It is similar with being petty in code review - sometimes people do it because they think it makes them look like better coders.

As for checking business case, yes sometimes you have to do it. But there is also such a thing as doing it too often and second guessing every requirement that you see during code review. This has tricle down effect on analysts/managers/or even customers (he got pissed after a while of this - true story) who then have to litigate and defend every detail and solve programmers conflicts over insignificant details.

Re: How I review code

#12

Post by skgoa »

gizmo686 wrote: post #4
anameaname wrote: post #3

A conundrum for me is how to get other people to code review the way I want to be code reviewed? Particularly, I noticed code reviewers on my team are pretty pedantic, obsessed with correctness, and need to be explained why each change is okay. These are people that regularly write good quality code themselves, but there is a high amount of distrust. Why doesn't a team of talented programmers trust each other? (in ca…

>obsessed with correctness, and need to be explained why each change is okay. Isn't this the point of code review? When I click accept on a code review, I am saying that I have looked over the change and believe that it is correct and okay. If I just arrive at the conclusion by saying "Joe wrote this, and I trust Joe" the there is no point in me reviewing it.

Proving correctness is not the point of a code review. In fact it would be difficult to make such a proof in sufficiently complex software. Functional correctness is typically "proven" by tests.

A code review ensures that the non-functional quality of the code is high. I.e. that the code is understandable/maintainable by someone other than the programmer himself, that there are no anti-patterns or dangerous-but-correct usages of language features, that the implementation fits to the intent of the specification etc.

Re: How I review code

#13

Post by jt2190 »

gizmo686 wrote: post #10
watwut wrote: post #9

Earlier quoted context omitted.

It continues with "and need to be explained why each change is okay". If reviewer suspects there is a bug, reviewer should check for it instead of having reviewee explain him or defend every detail. It is micromanagement this way - not just being similar or kinda like micromanagement, but it is literally it. If every five lines big code change in run of the mill fronted requires two hours long negotiation, then somet…

>If reviewer suspects there is a bug, reviewer should check for it instead of having reviewee explain him or defend every detail. It is not a matter of if the reviewer "suspects there is a bug", but rather a matter of if the reviewer is convinced that there is not a bug. If the reviewer needs to have someone explain why every minor code change is correct, they are either lazy or incompetent (or the code is badly writ…

> However, being "okay" often goes beyond correctness, and into business decisions.

Code reviews occur far too late in the development process for an uninvolved developer to provide a good, timely review of “bigger picture” concerns like business consideration (or software design) simply because the reviewing developer needs time to come up to speed on why decisions were made. In these cases it’s better to have the reviewing developer involved in a review of business requirements before the code is written. The code review can then stay focused on code.

Re: How I review code

#14

Post by adrianN »

anameaname wrote: post #3

A conundrum for me is how to get other people to code review the way I want to be code reviewed? Particularly, I noticed code reviewers on my team are pretty pedantic, obsessed with correctness, and need to be explained why each change is okay. These are people that regularly write good quality code themselves, but there is a high amount of distrust. Why doesn't a team of talented programmers trust each other? (in ca…

I'm an overly pedantic reviewer because I feel the review is one of the few places where I can counteract the accumulation of technical debt. Once the less than ideal code is in the codebase it's there to stay. I also ask a lot of pretty dumb questions during a review because I want to make sure that my understanding of the requirements matches the understanding the other engineer had.

Re: How I review code

#15

Post by deepaksurti »

gelo wrote: post #5
anameaname wrote: post #3

A conundrum for me is how to get other people to code review the way I want to be code reviewed? Particularly, I noticed code reviewers on my team are pretty pedantic, obsessed with correctness, and need to be explained why each change is okay. These are people that regularly write good quality code themselves, but there is a high amount of distrust. Why doesn't a team of talented programmers trust each other? (in ca…

Ive had this pedantic-ness in the past. I've noticed the obsessive compulsive behaviour of being attached to code. People I had to work with cared more about having every for-loop contain a ++i instead of i++ when generally most compilers optimize for that. They ignored the fact that I pointed out a problem in their architecture and basically told me to get on with it. If someone is attached to code and takes offence…

>> they shouldn't be in the job

or may be we should find a job where constructive criticism is part of the culture and you then leave the current job! Relatively easier to find where the change exists than investing effort to bring about a change.

Re: How I review code

#16

Post by xerophyte12932 »

Vinnl wrote: post #7
anameaname wrote: post #3

A conundrum for me is how to get other people to code review the way I want to be code reviewed? Particularly, I noticed code reviewers on my team are pretty pedantic, obsessed with correctness, and need to be explained why each change is okay. These are people that regularly write good quality code themselves, but there is a high amount of distrust. Why doesn't a team of talented programmers trust each other? (in ca…

If you can't see why a line of code is okay without looking at it, doesn't that mean that it might need a comment or something explaining why it is? As a corollary, when I ask a question in code review, I usually don't want it to be answered there - I'd prefer it to be answered in the code, if need be using a comment.

Exactly, because you don't want every future reader of the code to have the same question.

This is why I hate implicit assumptions. If I need some special knowledge or some special assumption that is no where in the immediate vicinity of the code, then maybe your approach to the problem is flawed. Sure it'll work but good luck to future coders working with it

Re: How I review code

#17

Post by xerophyte12932 »

yeukhon wrote: post #2

I echo the author's point in "Review the code with its author in mind". Without comments, sometimes it's really really difficult to navigate the code. I have been adding more comments than ever: don't assume every line is obvious, write a comment to explain what the next few lines really do. # base case: stop dividing when we find the largest square. if width == height: return width, height else: # otherwise, we know…

A good counter argument to Comments:

https://blog.codinghorror.com/coding-without-comments/

Re: How I review code

#18

Post by carlmr »

adrianN wrote: post #14
anameaname wrote: post #3

A conundrum for me is how to get other people to code review the way I want to be code reviewed? Particularly, I noticed code reviewers on my team are pretty pedantic, obsessed with correctness, and need to be explained why each change is okay. These are people that regularly write good quality code themselves, but there is a high amount of distrust. Why doesn't a team of talented programmers trust each other? (in ca…

I'm an overly pedantic reviewer because I feel the review is one of the few places where I can counteract the accumulation of technical debt. Once the less than ideal code is in the codebase it's there to stay. I also ask a lot of pretty dumb questions during a review because I want to make sure that my understanding of the requirements matches the understanding the other engineer had.

Exactly, just sounds like good review practices to me.

Re: How I review code

#19

Post by Stratoscope »

xerophyte12932 wrote: post #17
yeukhon wrote: post #2

I echo the author's point in "Review the code with its author in mind". Without comments, sometimes it's really really difficult to navigate the code. I have been adding more comments than ever: don't assume every line is obvious, write a comment to explain what the next few lines really do. # base case: stop dividing when we find the largest square. if width == height: return width, height else: # otherwise, we know…

A good counter argument to Comments: https://blog.codinghorror.com/coding-without-comments/

That article doesn't make a compelling case.

It suggests taking this code:

  // square root of n with Newton-Raphson approximation
  r = n / 2;
  while ( abs( r - (n/r) ) > t ) {
      r = 0.5 * ( r + (n/r) );
  }

  System.out.println( "r = " + r );
And refactoring it to this function:

  private double SquareRootApproximation(n) {
      r = n / 2;
      while ( abs( r - (n/r) ) > t ) {
          r = 0.5 * ( r + (n/r) );
      }
      return r;
  }

  System.out.println( "r = " + SquareRootApproximation(r) );
I'm all for this refactoring, but something was lost in the process. What kind of square root approximation is being used? Does the algorithm have a name? What would I search for if I wanted to read more about it? That information was in the original comment.

Re: How I review code

#20

Post by yeukhon »

xerophyte12932 wrote: post #17
yeukhon wrote: post #2

I echo the author's point in "Review the code with its author in mind". Without comments, sometimes it's really really difficult to navigate the code. I have been adding more comments than ever: don't assume every line is obvious, write a comment to explain what the next few lines really do. # base case: stop dividing when we find the largest square. if width == height: return width, height else: # otherwise, we know…

A good counter argument to Comments: https://blog.codinghorror.com/coding-without-comments/

I won't speak for anyone else, but I've written some really good code but then I had a hard time understand what the heck I did after a year.

Joe's arguments are weak

* code should be readable * good comments require good writers * refactoring * His example is way too simple. Try a more difficult approximation function.

Joe needs to realize that code produced at work is not meant for one single individual. If I leave, I want my co-workers and their future co-workers to have a good time navigate through codebase.

Use comments wisely, but don't avoid them! Adding 10 extra lines of comments to the file is better than a one-liner no one can understand. Let's not run a 100-line competition when we are writing code professionally. I shouldn't need to frustrate my reviewers and beg me to explain. Use newlines to make your code more readable as well.

Post reply on HN