10 ms·

Code Review Best Practices

221 points by Kaedon 11y ago

a3voices 11y ago
Having worked for businesses that use code reviews and those that don't, I personally favor not having code reviews. The reason is that they hinder development speed quite a lot, since you have to try to predict what other engineers will say on your reviews, which takes a lot of brainpower.
- jdcantrell 11y ago
  I think code reviews are critical for sharing knowledge on team projects. It helps keep your team informed about refactors and new functionality, while also giving a space for feedback on implementation (in a critical time, before the code has shipped). It allows a very organic way for people to learn from others (reading code, asking about code, thinking about others' code). That said, you have to get useful code reviews to see any benefit. To me that means using automated tools to do most of the style checks (your braces should be on this line, no space after this foreach, etc) and having an active culture of not being human-powered code linters when doing code reviews. There is a lot of work that goes into having a team give effective code reviews. I agree that code reviews can slow down an individual, but the speed up to the team through shared understanding should make up for that.
- matwood 11y ago
  > since you have to try to predict what other engineers will say on your reviews So the code is written with higher quality up front? Sounds like reviews are working as intended.
  - tomjen3 11y ago
    If so, don't you think that is what he would have written? There are enough differences over what can be good quality that teams can be bogged down over discussions that just have no good answer and these take a lot of brain power.
- jlarocco 11y ago
  Also having experienced both, I wouldn't want to work in a place that didn't have code reviews. I ask myself, "WTF was this guy thinking when he wrote this?" far less often when I'm working on code that's been code reviewed. Most of the time, the reviewer catches code like that, and requests that it be fixed and/or commented, before I end up debugging through it 6 months later. IMO, open office plans, reading HN and nerf guns hinder development speed far more than code reviews.
  - MaulingMonkey 11y ago
    I experienced both within the same company - they started without code reviews, and the later introduced them. The result was overwhelmingly positive, greatly improving code quality and helping spread knowledge of new systems and utilities. I certainly believe it's possible to do code reviews sufficiently "wrong" that they're a net hindrance. Drape them in too much ceremony, put too much emphasis on style (Bob doesn't like your whitespace preferences, and he's the code owner, so your changelist is vetoed!) and too little emphasis on subsistence (these corner cases and possible bugs concern me). . We went with a relatively light touch approach. Nothing was technologically enforced - our lead dev simply told us he wanted everyone to start having everything reviewed by anyone - both for code quality and to spread knowledge - and that he'd be pissed if he found a bug in one of our changelists and found out it hadn't been reviewed, going forward. Whitespace / naming stuff I tended to submit without review. Quick and obvious fixes, small changes, etc., I was fairly willing to start a review request, submit, and then fix anything caught in the review in a followup changelist. For larger changes where I had more reservations, I'd usually hold off on submitting until after a review. Maybe threaten to submit if they were dragging their feet too long :P
- jrbancel 11y ago
  I agree that requiring deep code reviews on every code change is ridiculous. Unfortunately, I have never seen a team that doesn't have at least a developer that is much worse than the average member of the team. He can be bad at abstraction, at design, at naming things or anything else. This developer produces sub-optimal (or even worse, incorrect) code that is hard to understand, maintain and extend. I have observed that naturally, the other teammates will review the code of this specific coworker while not looking at changes submitted by good and reliable teammates. I see code reviews as a tool to provide feedbacks to someone and help him improve the quality of the code he writes.
bozoUser 11y ago
While I agree with all the points in the blog, I wonder how many programmers do really follow them perfectly(even the author of the blog) because doing code review to such great detail requires plenty of time which is often not the case when you work for corporations.
- Kaedon 11y ago
  I'll admit that I don't follow them perfectly either. I'll usually pick one or two instances and work on those. I think most of these items are on a continuum from severe to minor basically. It's usually when it's on the severe end that I'll suggest it might be worth changing. My code has bugs and flaws too! That's why I like code reviews, they give me an opportunity to see my blind spots.
- chunkstuntman 11y ago
  One company I worked for relied heavily on code reviews after every feature. At least two co-workers (one of whom had to be a supervisor) read, ran, and gave feedback on every piece of code. Reading others' code and providing feedback allowed me to improve my sight-linting ability, and it felt like each day my group's code as a whole was improving. Having some accountability for writing sloppy code is very sobering.
  - clebio 11y ago
    > supervisor ... ran ... code This and this again. Engineering managers, and all that. But also, don't just review, 'sight-lint', and reason about code. Rather, run the tests! It's the shared accountability and knowledge. Does the baseline capability exist (tests pass)? Can you, the reviewer, spot fallacies that the tests don't capture (if not, criticisms feed back to you later)?
BurningFrog 11y ago
This is not a bad list, and can be useful as a checklist. But note that it basically tries to define good programming practice in general. That's a very big topic with a lot of room for debate and disagreement.
cmpb 11y ago
Nice list. This is more or less what I look for. It's nice to see your rules of thumb. Anyone have any suggestions for time-estimating code review? That's the biggest issue we've faced trying to implement code review into our workflow.
- Kaedon 11y ago
  SmartBear has their own set of best practices and they recommend about 300-400 loc per hour[1]. I think that's probably about right. It's something I think I've gotten faster at over time, partially because we're improving from previous reviews and partially because it's a skill that develops. [1]: http://smartbear.com/smartbear/media/pdfs/wp-cc-11-best-practices-of-peer-code-review.pdf http://smartbear.com/smartbear/media/pdfs/wp-cc-11-best-prac...
deleted 11y ago
[deleted]
- deleted 11y ago
  [deleted]
- deleted 11y ago
  [deleted]
enqk 11y ago
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.html http://number-none.com/blow/john_carmack_on_inlined_code.htm...
- hueving 11y ago
  I disagree. Breaking down a function into several one time functions that all sit at the same mental model of abstraction make code much easier to reason about.
  - Fr0styMatt8 11y ago
    I agree with you in principle but find that code editors let me down in that regard. Say I split a function up into a few sub-functions because it's getting big. Now I have the problem that I'm jumping forwards and backwards through the code when I want to explore what that function does: SomeMassiveFunction() { SubfunctionA(); SubfunctionB(); SubfunctionC(); SubfunctionD(); } SubfunctionA() { } .... In this case, SubfunctionC() might end up a few pages down in source code. So now it's a context switch to go there and then go back. Now this can be somewhat avoided with good function names (so you don't HAVE to jump backwards and forwards) and keyboard shortcuts (to make the process quicker), but it's still a trade-off that I think needs to be kept in mind.
    - ams6110 11y ago
      When I have to deal with something like this in emacs I will normally split the window in two so I can look at two different parts of the buffer at the same time. I assume most other editors allow the same?
    - samspot 11y ago
      I think you've hit upon a good measure of function quality. If you find you have to jump around a lot when reading, that would be a sign that it's poorly organized and needs to be refactored. On the other hand, if you find you don't have to jump around and can trust the sub functions by their names, then it's been broken up well. In the best case you should be able to follow the logic without diving into the other functions, only looking at their implementation details as that particular detail becomes relevant.
zatkin 11y ago
Awesome. I'm joining Cisco for the summer, so I think this would help me get a head start since they do code review. Thank you!
- azatris 11y ago
  Are you the person who took my place? :) Got to the last stage, twice, in London. To me, the Code Review Best Practices seem awfully like very general knowledge, just gathered together. Not sure if it's HN-worthy per se. However, the John Carmack link is quite enlightening.
  - zatkin 11y ago
    I sure hope not -- I'm working at the headquarters in San Jose this summer.
- Kaedon 11y ago
  Sure! Best of luck at Cisco. I think everyone looks for different things in a code review, this is just what has worked for me.
trustfundbaby 11y ago
> If the reviewer makes a suggestion, and I don’t have a clear answer as to why the suggestion should not be implemented, I’ll usually make the change This I feel is bad. Code reviews are usually between peers so you shouldn't be afraid to seek out clarification where possible. You shouldn't be making edits to code that goes in production without clearly understanding why. The other thing that wasn't mentioned, that I think is important, is to not act as a blocker for code reviews unless its absolutely necessary. Lots of engineers take on the attitude that they're going to "gate" code they don't agree with by with holding their +1 and bogging down the review with questions and all sorts of runarounds till its what they want. this is a bad attitude to have, even when you're dealing with Junior engineers. I'm generally going to +1 something unless I fundamentally disagree with it or think its going to break things in production. What I do, though, is leave lots of comments with questions/suggestions and mention it in the +1 with (see comments). This builds trust on teams, and stops things getting personal, especially with people who aren't very good at dealing with criticism, even in something as banal as a CR. On a team that works well together, teammates will see those comments, think about them and make thoughtful responses, especially once they understand that you're not trying to get in their way. Giving the +1 gives them the freedom to consider your suggestions without being irritated that their PR is being blocked. They feel like they're in control not you. In rare exceptions, someone will brush off my questions and merge ... which means that next time, I get to be tougher on the review and specifically ask for responses before the code can be merged, because they've degraded the implicit team trust. Usually repeat offenders are assholes, and assholes generally don't last on healthy teams.
- allsystemsgo 11y ago
  I agree. I have had review comments that have made no sense at all. If I didn't ask why, I wouldn't learn. Also, to be honest, there have been times where the review comment didn't understand the context of my code, so the review comment ended up being incorrect.
- Cymen 11y ago
  I agree 100% with the "don't be a blocker." I personally look for about 80-90% yes and if it meets that criteria with no errors, I thumb it up. If I can't thumb it up, I try to put a "thumbs up with commented items fixed". I try to not block -- comment for bad issues, sometimes sigh but agree when it's a 10% disagreement and go forward. Life is a series of iterations. That 10% will be addressed in a future iteration. And I never close another persons PR.
USNetizen 11y ago
The one thing that is missing, which ALWAYS seems to fall by the wayside, is security. If people incorporated more iterative security testing (static AND dynamic, automated AND manual) and threat modeling into their SDLC reviews there would be a plummeting number of vulnerabilities. But, because it doesn't fit in with the whole "Lean" approach to software (deliver features yesterday), all but the most established enterprises don't seem to care much unfortunately. Once more people experience a breach because of their desire to deliver first and remediate vulnerabilities later then perhaps more awareness will be raised. By then it's too late though.
maguirre 11y ago
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?
- shakeel_mohamed 11y ago
  +1 I'm currently dealing with this. The individual doesn't offer feedback on my code when under review, so it appears like a personal attack.
  - frossie 11y ago
    It is important that code reviews are public enough that people see other people review each other's code - a system where the only code reviews you see are the ones you do and the ones you get leads to poor expectations of what they are for. Having at least two very accomplished and (culturally and organisationally senior) people routinely "model" reasonable review behaviour can be stunningly effective. Also this is an area where frequent team conversations about what good code is outside a review situation helps to build a certain culture. It helps step away from nitpicking and arguing.
  - halostatue 11y ago
    Go have coffee (or whatever) with your colleague and have a 1-to-1 conversation with them saying you want their feedback on code. You want their expertise to make you a better software developer. Even if you are better at this than they are, you can learn something from the sharing. The team’s lead should make sure that the individual in question knows that software development is a cooperative process and that code review helps build the team.
    - MaulingMonkey 11y ago
      This. Although I'm less civil about it - I'll start giving people shit for rubber stamping my code reviews. "Why are you letting me ship this terrible code I wrote? Do you want us to end up in a death spiral of technical debt and crunch? Don't be an asshole, critique my shit! We all need a second set of eyes - I'm no exception!" ...okay, I'm maybe a little more civil than that. But I'm not above leaving some mistakes unfixed to call them out on.
shakeel_mohamed 11y ago
Another thing to check for that I didn't see addressed is debug statements. There's nothing quite like seeing console.log("shit"); during a code review :)
deleted 11y ago
[deleted]
bottled_poe 11y ago
Some of this is frustrating to read. Architectural and detailed design decisions should be made and approved almost entirely before the coding of those features is started. (Obviously this is the ideal and not always possible). This means that the code reviews should involve little more that a checklist of those approved design decisions against their implementation, perhaps a code style is verified as well. Coding without a design is just hacking, which I believe is the primary cause of burn-out and should be avoided as much as possible. So, the question is, do you have a design document? I know it doesn't sound very agile, but traditional engineering procedures, when managed well, have a lot of merit in controlling product quality and cost.
OnlineRevenge 11y ago
"Like anybody would be, I was very skeptical about using a love spell or any spell for that matter but I was absolutely shocked when Tim called me after I had you guys cast my "Return My Lover" spell for me. It wasn't 24 hours that I had my spell cast that he came back to me (practically on his knees). He broke up with me over a month ago and now we are happier than ever. Thank you all!" quickrevengespell@yahoo.com, Dorothy Rodriquez, New York
OnlineRevenge 11y ago
I want to testify of this great death spell caster. This great man helped me cast a death spell on my wicked step father and just within 48hours the wicked man had a motor crash and died. All thanks to this great death spell caster called instant death spell. You too can contact him now for an urgent death spell cast on anyone, quickrevengespell@yahoo.com,
- Kiro 11y ago
  This is the strangest spam I've ever seen on here.
OnlineRevenge 11y ago
"Like anybody would be, I was very skeptical about using a love spell or any spell for that matter but I was absolutely shocked when Tim called me after I had you guys cast my "Return My Lover" spell for me. It wasn't 24 hours that I had my spell cast that he came back to me (practically on his knees). He broke up with me over a month ago and now we are happier than ever. Thank you all!" quickrevengespell@yahoo.com, Dorothy Rodriquez, New York
OnlineRevenge 11y ago
"I missed my ex bad. My family and friends were tired of me being so upset one of them actually ordered a Love Spell for me From Extreme Spells I had no idea what they had done. They ordered the GLOBAL LOVE SPELL as it your best and most powerful and effective Love Spell. Needless to say, I was shocked to see my wife at the door a week later with her eyes full of tears, .I cannot believe how well my spell worked. I recently ordered a Money Spell because who doesn't need extra money?" quickrevengespell@yahoo.com -- William, Nashville
Kiro 11y ago
> If we have to use “and” to finish describing what a method is capable of doing, it might be at the wrong level of abstraction. When I coee, somewhere a method needs to initiate this execution flow and will therefore contain "and". Even if all it does is call two other methods where this principle is followed. How do I avoid this? I mean, somewhere in the code it makes sense to execute the methods together.
justinfreitag 11y ago
Style, complexity and coverage checks should be left to automated tooling. Code reviews should focus on whatever remains.
pablobaz 11y ago
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 it. This leads to a focus on relatively trivial matters such as naming conventions and method lengths. I think that the underlying reason for this is laziness. Talking about clever refactoring is an easier/faster process than understanding the 'why'.
- yoran 11y ago
  I agree with this. But I don't think code reviews are suited for this step 0 as you say. Just the way a pull request is formatted, it's very hard for the reviewer to deduce from the changeset what the high-level design of the code is. That's why we discuss these high-level design and architectural decisions before-hand so that they are known to the reviewer at the time of the review. We're a small team so it works well. But I'm not sure how this scales up as the team gets bigger. I would like to know how bigger teams approach this problem!
- jeremiep 11y ago
  Same here, I've seen too many code reviews where people complain about a badly named variables and nobody saw the design was faulty leading to costly bugs to fix in production. One thing we did on the current project is pre-commit reviews in pairs. This ensures at least two people in the team knows about the changes, let us talk about the why and how of the changes, and possibly teach a coworker new things in the process. What it ended up doing is that every programmer now self-reviews their own changes prior to the actual review knowing they'll soon share it all face to face with a coworker. Turns out the talks are now about the design of the code, not how it looks.
- MichaelGG 11y ago
  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?
mikehaggard 11y ago
>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.
- MichaelGG 11y ago
  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.
Too 11y ago
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?
OnlineRevenge 11y ago
HERE COMES THE MASTER OF DEATH SPELLS. THIS GREAT DEATH SPELL CASTER CALLED "REVENGE DEATH SPELL" IS TRULY A GREAT DEATH SPELL CASTER INDEED.HE HELPED ME CAST A DEATH SPELL ON MY WICKED AND HEARTLESS EX HUSBAND AND JUST WITHIN 24HOURS THE BASTARD WAS CONFIRMED DEAD IN HIS SLEEPING BED. IF YOU ALSO NEED AN URGENT DEATH SPELL ON ANYONE THAT CONTACT THIS GREAT DEATH SPELL CASTER IMMEDIATELY VIA EMAIL, quickrevengespell@yahoo.com, website: www.quickrevengespells.com

221 points by Kaedon 11y ago

Having worked for businesses that use code reviews and those that don't, I personally favor not having code reviews. The reason is that they hinder development speed quite a lot, since you have to try to predict what other engineers will say on your reviews, which takes a lot of brainpower.

jdcantrell 11y ago
I think code reviews are critical for sharing knowledge on team projects. It helps keep your team informed about refactors and new functionality, while also giving a space for feedback on implementation (in a critical time, before the code has shipped). It allows a very organic way for people to learn from others (reading code, asking about code, thinking about others' code). That said, you have to get useful code reviews to see any benefit. To me that means using automated tools to do most of the style checks (your braces should be on this line, no space after this foreach, etc) and having an active culture of not being human-powered code linters when doing code reviews. There is a lot of work that goes into having a team give effective code reviews. I agree that code reviews can slow down an individual, but the speed up to the team through shared understanding should make up for that.
matwood 11y ago
> since you have to try to predict what other engineers will say on your reviews So the code is written with higher quality up front? Sounds like reviews are working as intended.
- tomjen3 11y ago
  If so, don't you think that is what he would have written? There are enough differences over what can be good quality that teams can be bogged down over discussions that just have no good answer and these take a lot of brain power.
jlarocco 11y ago
Also having experienced both, I wouldn't want to work in a place that didn't have code reviews. I ask myself, "WTF was this guy thinking when he wrote this?" far less often when I'm working on code that's been code reviewed. Most of the time, the reviewer catches code like that, and requests that it be fixed and/or commented, before I end up debugging through it 6 months later. IMO, open office plans, reading HN and nerf guns hinder development speed far more than code reviews.
- MaulingMonkey 11y ago
  I experienced both within the same company - they started without code reviews, and the later introduced them. The result was overwhelmingly positive, greatly improving code quality and helping spread knowledge of new systems and utilities. I certainly believe it's possible to do code reviews sufficiently "wrong" that they're a net hindrance. Drape them in too much ceremony, put too much emphasis on style (Bob doesn't like your whitespace preferences, and he's the code owner, so your changelist is vetoed!) and too little emphasis on subsistence (these corner cases and possible bugs concern me). . We went with a relatively light touch approach. Nothing was technologically enforced - our lead dev simply told us he wanted everyone to start having everything reviewed by anyone - both for code quality and to spread knowledge - and that he'd be pissed if he found a bug in one of our changelists and found out it hadn't been reviewed, going forward. Whitespace / naming stuff I tended to submit without review. Quick and obvious fixes, small changes, etc., I was fairly willing to start a review request, submit, and then fix anything caught in the review in a followup changelist. For larger changes where I had more reservations, I'd usually hold off on submitting until after a review. Maybe threaten to submit if they were dragging their feet too long :P
jrbancel 11y ago
I agree that requiring deep code reviews on every code change is ridiculous. Unfortunately, I have never seen a team that doesn't have at least a developer that is much worse than the average member of the team. He can be bad at abstraction, at design, at naming things or anything else. This developer produces sub-optimal (or even worse, incorrect) code that is hard to understand, maintain and extend. I have observed that naturally, the other teammates will review the code of this specific coworker while not looking at changes submitted by good and reliable teammates. I see code reviews as a tool to provide feedbacks to someone and help him improve the quality of the code he writes.

bozoUser 11y ago

While I agree with all the points in the blog, I wonder how many programmers do really follow them perfectly(even the author of the blog) because doing code review to such great detail requires plenty of time which is often not the case when you work for corporations.

Kaedon 11y ago
I'll admit that I don't follow them perfectly either. I'll usually pick one or two instances and work on those. I think most of these items are on a continuum from severe to minor basically. It's usually when it's on the severe end that I'll suggest it might be worth changing. My code has bugs and flaws too! That's why I like code reviews, they give me an opportunity to see my blind spots.
chunkstuntman 11y ago
One company I worked for relied heavily on code reviews after every feature. At least two co-workers (one of whom had to be a supervisor) read, ran, and gave feedback on every piece of code. Reading others' code and providing feedback allowed me to improve my sight-linting ability, and it felt like each day my group's code as a whole was improving. Having some accountability for writing sloppy code is very sobering.
- clebio 11y ago
  > supervisor ... ran ... code This and this again. Engineering managers, and all that. But also, don't just review, 'sight-lint', and reason about code. Rather, run the tests! It's the shared accountability and knowledge. Does the baseline capability exist (tests pass)? Can you, the reviewer, spot fallacies that the tests don't capture (if not, criticisms feed back to you later)?

BurningFrog 11y ago

This is not a bad list, and can be useful as a checklist. But note that it basically tries to define good programming practice in general. That's a very big topic with a lot of room for debate and disagreement.

cmpb 11y ago

Nice list. This is more or less what I look for. It's nice to see your rules of thumb. Anyone have any suggestions for time-estimating code review? That's the biggest issue we've faced trying to implement code review into our workflow.

Kaedon 11y ago
SmartBear has their own set of best practices and they recommend about 300-400 loc per hour[1]. I think that's probably about right. It's something I think I've gotten faster at over time, partially because we're improving from previous reviews and partially because it's a skill that develops. [1]: http://smartbear.com/smartbear/media/pdfs/wp-cc-11-best-practices-of-peer-code-review.pdf http://smartbear.com/smartbear/media/pdfs/wp-cc-11-best-prac...

deleted 11y ago

[deleted]

deleted 11y ago
[deleted]
deleted 11y ago
[deleted]

enqk 11y ago

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.html http://number-none.com/blow/john_carmack_on_inlined_code.htm...

hueving 11y ago
I disagree. Breaking down a function into several one time functions that all sit at the same mental model of abstraction make code much easier to reason about.
- Fr0styMatt8 11y ago
  I agree with you in principle but find that code editors let me down in that regard. Say I split a function up into a few sub-functions because it's getting big. Now I have the problem that I'm jumping forwards and backwards through the code when I want to explore what that function does: SomeMassiveFunction() { SubfunctionA(); SubfunctionB(); SubfunctionC(); SubfunctionD(); } SubfunctionA() { } .... In this case, SubfunctionC() might end up a few pages down in source code. So now it's a context switch to go there and then go back. Now this can be somewhat avoided with good function names (so you don't HAVE to jump backwards and forwards) and keyboard shortcuts (to make the process quicker), but it's still a trade-off that I think needs to be kept in mind.
  - ams6110 11y ago
    When I have to deal with something like this in emacs I will normally split the window in two so I can look at two different parts of the buffer at the same time. I assume most other editors allow the same?
  - samspot 11y ago
    I think you've hit upon a good measure of function quality. If you find you have to jump around a lot when reading, that would be a sign that it's poorly organized and needs to be refactored. On the other hand, if you find you don't have to jump around and can trust the sub functions by their names, then it's been broken up well. In the best case you should be able to follow the logic without diving into the other functions, only looking at their implementation details as that particular detail becomes relevant.

zatkin 11y ago

Awesome. I'm joining Cisco for the summer, so I think this would help me get a head start since they do code review. Thank you!

azatris 11y ago
Are you the person who took my place? :) Got to the last stage, twice, in London. To me, the Code Review Best Practices seem awfully like very general knowledge, just gathered together. Not sure if it's HN-worthy per se. However, the John Carmack link is quite enlightening.
- zatkin 11y ago
  I sure hope not -- I'm working at the headquarters in San Jose this summer.
Kaedon 11y ago
Sure! Best of luck at Cisco. I think everyone looks for different things in a code review, this is just what has worked for me.

trustfundbaby 11y ago

> If the reviewer makes a suggestion, and I don’t have a clear answer as to why the suggestion should not be implemented, I’ll usually make the change This I feel is bad. Code reviews are usually between peers so you shouldn't be afraid to seek out clarification where possible. You shouldn't be making edits to code that goes in production without clearly understanding why. The other thing that wasn't mentioned, that I think is important, is to not act as a blocker for code reviews unless its absolutely necessary. Lots of engineers take on the attitude that they're going to "gate" code they don't agree with by with holding their +1 and bogging down the review with questions and all sorts of runarounds till its what they want. this is a bad attitude to have, even when you're dealing with Junior engineers. I'm generally going to +1 something unless I fundamentally disagree with it or think its going to break things in production. What I do, though, is leave lots of comments with questions/suggestions and mention it in the +1 with (see comments). This builds trust on teams, and stops things getting personal, especially with people who aren't very good at dealing with criticism, even in something as banal as a CR. On a team that works well together, teammates will see those comments, think about them and make thoughtful responses, especially once they understand that you're not trying to get in their way. Giving the +1 gives them the freedom to consider your suggestions without being irritated that their PR is being blocked. They feel like they're in control not you. In rare exceptions, someone will brush off my questions and merge ... which means that next time, I get to be tougher on the review and specifically ask for responses before the code can be merged, because they've degraded the implicit team trust. Usually repeat offenders are assholes, and assholes generally don't last on healthy teams.

allsystemsgo 11y ago
I agree. I have had review comments that have made no sense at all. If I didn't ask why, I wouldn't learn. Also, to be honest, there have been times where the review comment didn't understand the context of my code, so the review comment ended up being incorrect.
Cymen 11y ago
I agree 100% with the "don't be a blocker." I personally look for about 80-90% yes and if it meets that criteria with no errors, I thumb it up. If I can't thumb it up, I try to put a "thumbs up with commented items fixed". I try to not block -- comment for bad issues, sometimes sigh but agree when it's a 10% disagreement and go forward. Life is a series of iterations. That 10% will be addressed in a future iteration. And I never close another persons PR.

USNetizen 11y ago

The one thing that is missing, which ALWAYS seems to fall by the wayside, is security. If people incorporated more iterative security testing (static AND dynamic, automated AND manual) and threat modeling into their SDLC reviews there would be a plummeting number of vulnerabilities. But, because it doesn't fit in with the whole "Lean" approach to software (deliver features yesterday), all but the most established enterprises don't seem to care much unfortunately. Once more people experience a breach because of their desire to deliver first and remediate vulnerabilities later then perhaps more awareness will be raised. By then it's too late though.

maguirre 11y ago

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?

shakeel_mohamed 11y ago
+1 I'm currently dealing with this. The individual doesn't offer feedback on my code when under review, so it appears like a personal attack.
- frossie 11y ago
  It is important that code reviews are public enough that people see other people review each other's code - a system where the only code reviews you see are the ones you do and the ones you get leads to poor expectations of what they are for. Having at least two very accomplished and (culturally and organisationally senior) people routinely "model" reasonable review behaviour can be stunningly effective. Also this is an area where frequent team conversations about what good code is outside a review situation helps to build a certain culture. It helps step away from nitpicking and arguing.
- halostatue 11y ago
  Go have coffee (or whatever) with your colleague and have a 1-to-1 conversation with them saying you want their feedback on code. You want their expertise to make you a better software developer. Even if you are better at this than they are, you can learn something from the sharing. The team’s lead should make sure that the individual in question knows that software development is a cooperative process and that code review helps build the team.
  - MaulingMonkey 11y ago
    This. Although I'm less civil about it - I'll start giving people shit for rubber stamping my code reviews. "Why are you letting me ship this terrible code I wrote? Do you want us to end up in a death spiral of technical debt and crunch? Don't be an asshole, critique my shit! We all need a second set of eyes - I'm no exception!" ...okay, I'm maybe a little more civil than that. But I'm not above leaving some mistakes unfixed to call them out on.

shakeel_mohamed 11y ago

Another thing to check for that I didn't see addressed is debug statements. There's nothing quite like seeing console.log("shit"); during a code review :)

deleted 11y ago

[deleted]

bottled_poe 11y ago

Some of this is frustrating to read. Architectural and detailed design decisions should be made and approved almost entirely before the coding of those features is started. (Obviously this is the ideal and not always possible). This means that the code reviews should involve little more that a checklist of those approved design decisions against their implementation, perhaps a code style is verified as well. Coding without a design is just hacking, which I believe is the primary cause of burn-out and should be avoided as much as possible. So, the question is, do you have a design document? I know it doesn't sound very agile, but traditional engineering procedures, when managed well, have a lot of merit in controlling product quality and cost.

OnlineRevenge 11y ago

"Like anybody would be, I was very skeptical about using a love spell or any spell for that matter but I was absolutely shocked when Tim called me after I had you guys cast my "Return My Lover" spell for me. It wasn't 24 hours that I had my spell cast that he came back to me (practically on his knees). He broke up with me over a month ago and now we are happier than ever. Thank you all!" quickrevengespell@yahoo.com, Dorothy Rodriquez, New York

OnlineRevenge 11y ago

I want to testify of this great death spell caster. This great man helped me cast a death spell on my wicked step father and just within 48hours the wicked man had a motor crash and died. All thanks to this great death spell caster called instant death spell. You too can contact him now for an urgent death spell cast on anyone, quickrevengespell@yahoo.com,

Kiro 11y ago
This is the strangest spam I've ever seen on here.

OnlineRevenge 11y ago

"I missed my ex bad. My family and friends were tired of me being so upset one of them actually ordered a Love Spell for me From Extreme Spells I had no idea what they had done. They ordered the GLOBAL LOVE SPELL as it your best and most powerful and effective Love Spell. Needless to say, I was shocked to see my wife at the door a week later with her eyes full of tears, .I cannot believe how well my spell worked. I recently ordered a Money Spell because who doesn't need extra money?" quickrevengespell@yahoo.com -- William, Nashville

Kiro 11y ago

> If we have to use “and” to finish describing what a method is capable of doing, it might be at the wrong level of abstraction. When I coee, somewhere a method needs to initiate this execution flow and will therefore contain "and". Even if all it does is call two other methods where this principle is followed. How do I avoid this? I mean, somewhere in the code it makes sense to execute the methods together.

justinfreitag 11y ago

Style, complexity and coverage checks should be left to automated tooling. Code reviews should focus on whatever remains.

pablobaz 11y ago

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 it. This leads to a focus on relatively trivial matters such as naming conventions and method lengths. I think that the underlying reason for this is laziness. Talking about clever refactoring is an easier/faster process than understanding the 'why'.

yoran 11y ago
I agree with this. But I don't think code reviews are suited for this step 0 as you say. Just the way a pull request is formatted, it's very hard for the reviewer to deduce from the changeset what the high-level design of the code is. That's why we discuss these high-level design and architectural decisions before-hand so that they are known to the reviewer at the time of the review. We're a small team so it works well. But I'm not sure how this scales up as the team gets bigger. I would like to know how bigger teams approach this problem!
jeremiep 11y ago
Same here, I've seen too many code reviews where people complain about a badly named variables and nobody saw the design was faulty leading to costly bugs to fix in production. One thing we did on the current project is pre-commit reviews in pairs. This ensures at least two people in the team knows about the changes, let us talk about the why and how of the changes, and possibly teach a coworker new things in the process. What it ended up doing is that every programmer now self-reviews their own changes prior to the actual review knowing they'll soon share it all face to face with a coworker. Turns out the talks are now about the design of the code, not how it looks.
MichaelGG 11y ago
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?

mikehaggard 11y ago

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

MichaelGG 11y ago
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.

Too 11y ago

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?

OnlineRevenge 11y ago

HERE COMES THE MASTER OF DEATH SPELLS. THIS GREAT DEATH SPELL CASTER CALLED "REVENGE DEATH SPELL" IS TRULY A GREAT DEATH SPELL CASTER INDEED.HE HELPED ME CAST A DEATH SPELL ON MY WICKED AND HEARTLESS EX HUSBAND AND JUST WITHIN 24HOURS THE BASTARD WAS CONFIRMED DEAD IN HIS SLEEPING BED. IF YOU ALSO NEED AN URGENT DEATH SPELL ON ANYONE THAT CONTACT THIS GREAT DEATH SPELL CASTER IMMEDIATELY VIA EMAIL, quickrevengespell@yahoo.com, website: www.quickrevengespells.com