Code Review Best Practices • Hacker News Board

Re: Code Review Best Practices

#81

Post by mikehaggard » Sun, May 10, 2015, 12:22 PM UTC

>Variable names: foo or bar are probably not useful method names for data structures. e is similarly not useful when compared to exception. Be as verbose as you need (depending on the language). Expressive variable names make it easier to understand code when we have to revisit it later.

I so agree with this!

Properly named variables is perhaps THE first line of defense against bad code. Too many developers think they are concise and having little code if they only abbreviate their variable names enough.

Honestly, "em", "erg", "fc" and "sc" may make perfect sense to use, but it's a form of obfuscation to future developers (including your future self).

Other pet peeve; adding things to variable names that don't add anything meaningful.

E.g.

"usersList"

Does your code really care that it's a list? Should the reader be pointed at this each and every time. I much prefer just using:

"users"

Clear, to the point, and readable.

Re: Code Review Best Practices

#82

Post by mikehaggard » Sun, May 10, 2015, 12:29 PM UTC

enqk wrote: post #2

setting thresholds on method / class sizes seems quite arbitrary and potentially harmful. Splitting a method into n different ones, none of which is called more than once is setting up the code for opportunistic reuse and obscures it's true function. It's especially wrong if the code that was split is mutating / non pure functional. see http://number-none.com/blow/john_carmack_on_inlined_code.htm...

>setting thresholds on method / class sizes seems quite arbitrary and potentially harmful. Splitting a method into n different ones, none of which is called more than once is setting up the code for opportunistic reuse and obscures it's true function.

Sometimes, but in general I don't agree.

If the functions are made private and giving appropriate names than it's really a way of self-documenting code, which helps.

Suppose you have:

    some_function() {
        // Calculate interest
        // 100 lines of code

        // Apply tax
        // 120 lines of code

        // Store results
        // 30 lines of code
    }

Then the need to have those "banner comments" there already indicate it might be a good idea to decompose the function into:

    some_function() {
        calculate_interest(...);
        apply_tax(...);
        store_results(...);
    }

It's not just about reuse, but about documenting what each section of code does, and about limiting interactions between arbitrary sets of variables. Functions are create boundaries (assuming they don't intensively use global variables or class instance variables when part of a class)

Re: Code Review Best Practices

#83

Post by MichaelGG » Sun, May 10, 2015, 1:05 PM UTC

mikehaggard wrote: post #81

>Variable names: foo or bar are probably not useful method names for data structures. e is similarly not useful when compared to exception. Be as verbose as you need (depending on the language). Expressive variable names make it easier to understand code when we have to revisit it later. I so agree with this! Properly named variables is perhaps THE first line of defense against bad code. Too many developers think the…

It's not a form of obfuscation if there is clear and simple context. Catch(ex) is perfectly fine, and adding 7 letters does nothing but add noise. Similarly "var us = getUsers()" is clear - it's not one u, it's multiple, and makes sense to have a loop like "for u in us".

A better alternative if you find the code is still confusing is to add context by cutting a function up into inner functions. This is why I really hate working in languages that make it difficult to define little closures. In F#, I'll often end up with a couple of 1 or 2 line inner functions and it makes things much easier to read. This simply isn't practical in e.g. C#.

In exported function names and certain class names or modules, sure, a bit of verbosity might help. But inside a function it just make it visually harder to understand and I've rarely found it to be beneficial. There's going to be enough context to load into my brain inside a function accurate, and unfortunately I still subvocalize when reading code and all those extra syllables add up.

Additionally, removing letters means you need to split lines less based on length and focus more on when it makes sense.

Re: Code Review Best Practices

#84

Post by MichaelGG » Sun, May 10, 2015, 1:10 PM UTC

pablobaz wrote: post #74

While I agree with all the points listed in the article, it highlights for me a major problem of a lot of code reviews. Most code reviews seem to focus on: 1. Examining what the change does 2. Finding ways to make the change in a nicer way. E.g. Refactoring etc. This leaves out the key step 0 - what is actually trying to be achieved, does it need to be done and is there a better (maybe completely different) way to do…

I'm guilty of getting stuck up on trivial formatting issues. When someone pushes a commit that has random whitespace (trailing or arbitrary newlines all over, or just inconsistent spacing), it feels sloppy. Same for many other simple things. If the code is unnecessarily superficially ugly, it sets up a block in my mind that makes it harder to focus on the real issues.

Is it wrong to kick this stuff back and tell devs to make it pretty first?

Re: Code Review Best Practices

#85

Post by mmcconnell1618 » Sun, May 10, 2015, 1:19 PM UTC

maguirre wrote: post #38

I have an interesting problem. A co-worker of mine appears extremely sensitive to his code being reviewed and I honestly don't know how to deal with it. He feels attacked (and becomes defensive during code reviews) because the reviewers focus on the "bad things and mistakes" of his code instead of the accomplishments. Has anyone here dealt with similar issues during reviews?

Try taking power away from the reviewer to disarm the defense reaction. In Ed Catmul's Creativity, Inc.

(http://www.amazon.com/gp/aw/d/0812993012/ref=mp_s_a_1_1?qid=...)

he describes the Pixar "brain trust" which reviews early progress on movies with the director. They found directors would get defensive if they felt someone more powerful was telling them to make specific changes. Once they set some rules like "the director decides what, if anything gets changed" and "talk about what's not working but don't solve the problem for the director" things improved. Now instead of film veterans telling you your movie (code) stinks it becomes about opportunities to make things awesome.

Re: Code Review Best Practices

#86

Post by enqk » Sun, May 10, 2015, 1:48 PM UTC

mikehaggard wrote: post #82

enqk wrote: post #2

setting thresholds on method / class sizes seems quite arbitrary and potentially harmful. Splitting a method into n different ones, none of which is called more than once is setting up the code for opportunistic reuse and obscures it's true function. It's especially wrong if the code that was split is mutating / non pure functional. see http://number-none.com/blow/john_carmack_on_inlined_code.htm...

>setting thresholds on method / class sizes seems quite arbitrary and potentially harmful. Splitting a method into n different ones, none of which is called more than once is setting up the code for opportunistic reuse and obscures it's true function. Sometimes, but in general I don't agree. If the functions are made private and giving appropriate names than it's really a way of self-documenting code, which helps. Su…

This depends on the language capabilities. In the C/Lisp/ML families of languages at least you can create isolated scopes. It's unfortunately not true of Javascript (and Ruby?).

Re: Code Review Best Practices

#87

Post by mhomde » Sun, May 10, 2015, 2:07 PM UTC

enqk wrote: post #2

setting thresholds on method / class sizes seems quite arbitrary and potentially harmful. Splitting a method into n different ones, none of which is called more than once is setting up the code for opportunistic reuse and obscures it's true function. It's especially wrong if the code that was split is mutating / non pure functional. see http://number-none.com/blow/john_carmack_on_inlined_code.htm...

It's always a balance between "The Blob" and the "Poltergeist" anti-pattern, when in doubt go more towards borderline "The Blob".

I just say though it's very seldom that a huge class/method is justified. Usually there are plenty self-enclosed logic or reusable methods that can be broken out either to utility libraries or a separate class.

My rule of thumb is that you should be able to look at a method/class and understand what it does, and that diminishes quickly over a certain size.

Sure some logic might be in other classes but that's another layer of abstraction

Re: Code Review Best Practices

#88

Post by Too » Sun, May 10, 2015, 2:11 PM UTC

maguirre wrote: post #38

I have an interesting problem. A co-worker of mine appears extremely sensitive to his code being reviewed and I honestly don't know how to deal with it. He feels attacked (and becomes defensive during code reviews) because the reviewers focus on the "bad things and mistakes" of his code instead of the accomplishments. Has anyone here dealt with similar issues during reviews?

Ask your manager to make the review a formal and required part of the process, for everyone. That way everybody is treated equally. If you go ask your colleague out of the blue why his code looks the way it does he might get defensive and wonder why you were suspicious to review his code behind his back.

Re: Code Review Best Practices

#89

Post by Too » Sun, May 10, 2015, 2:31 PM UTC

This list is very very basic, most of the things like style shouldn't have to be discussed and design should preferably be done before the code is written.

Just adding "error handling and potential bugs" as a generic bullet on the list just doesn't cut it, these should basically be the only items on the list but specified in much greater detail. A serious code review checklist should contain concrete scenarios of these, preferably tailored for your specific application.

Examples of this are: What happens during system startup, before all other modules are ready to answer to requests? What happens if the user enters malformed data (sql injections etc)? Does this function shut down gracefully (transaction safe)? How will the algorithms scale? Race conditions and deadlocks? What happens if the network to the database goes down? Is localization handled properly? Backwards compatibility?

Re: Code Review Best Practices

#90

Post by dkubb » Sun, May 10, 2015, 2:33 PM UTC

MichaelGG wrote: post #84

pablobaz wrote: post #74

While I agree with all the points listed in the article, it highlights for me a major problem of a lot of code reviews. Most code reviews seem to focus on: 1. Examining what the change does 2. Finding ways to make the change in a nicer way. E.g. Refactoring etc. This leaves out the key step 0 - what is actually trying to be achieved, does it need to be done and is there a better (maybe completely different) way to do…

I'm guilty of getting stuck up on trivial formatting issues. When someone pushes a commit that has random whitespace (trailing or arbitrary newlines all over, or just inconsistent spacing), it feels sloppy. Same for many other simple things. If the code is unnecessarily superficially ugly, it sets up a block in my mind that makes it harder to focus on the real issues. Is it wrong to kick this stuff back and tell devs…

I believe that ugly code is usually buggy code. If someone didn't take the time to deal with trivial issues like formatting then it makes me wonder if they spent time thinking hard about the actual problem they were trying to solve.

I usually explicitly do code reviews in 2 passes. I look for low level problems first, and also try to identify ways that they can be tested using tools so that next time they are caught before the code is sent for review. My second pass looks at higher level problems with the overall design and checks the functionality to make sure it matches the objectives.