Comments on Running Code Reviews with Confidence

17 Reader Comments

Back to the Article
  1. I recently discovered tracking gets set up automatically if you checkout the branch ‘directly’:

    git checkout 61524-broken-link

    should be all you need.

    Copy & paste the code below to embed this comment.
  2. Just learned about git push origin --delete <branch> I’ve always done git push origin :<branch> which is a lot harder to understand (and remember). Also did some digging and found it was added in 1.7.0 (https://raw.githubusercontent.com/git/git/master/Documentation/RelNotes/1.7.0.txt)

    Copy & paste the code below to embed this comment.
  3. I have been using atlassians crucible/fisheye with in our team for peer reviews, and i must say these tools are nice. Readers should try out these tools, they will eliminate time lost in git commands and branching.

    Copy & paste the code below to embed this comment.
  4. Good advice.

    I’m a solo developer, and yet as I mature I’m getting good at self-reviews. I love the fact that JetBrains’s IDEs put a visual diff right in the check-in window. JetBrains is working on a tool specifically for code reviews; their intro video makes it look a lot like the things that are already in the IDEs.

    Code that doesn’t compile or which fails simple regression tests shouldn’t be reviewed. Accidents do happen, but don’t waste people’s time by (git) pushing obviously broken code.

    One thing I look for in particular is debug code and code that made sense only while debugging (e.g. simple variables that are only used once.) In part, that’s because it’s fairly easy to find [at least in my code :-( ] so it gives me something to focus on. Again, I’m doing self-reviews, so looking at other people’s code would be easier to look at with fresh eyes.

    Copy & paste the code below to embed this comment.
  5. Nice article. I am now suffering from code reviews stuff. I used a atlassians crucible/fisheye for code review, it’s save more time. Thanks. :)

    Copy & paste the code below to embed this comment.
  6. I fully agree that code reviews are an essential and useful part of any development workflow.  But I disagree about what you include in the code review process.

    Many of the steps you outline would be better performed prior to a review request, and in many cases via an automated process:

    (I) Stylistic adherence is better managed via a pre-commit lint, and any issues should prevent a commit without a specific override on the part of the developer.

    (II) Unit testing should also be performed prior to review, and rather than having the reviewer run a test himself it should be a policy of the review process that all review requests include the updated unit test source.  (So if I’m modifying utils.cc I’ve also updated and committed tests/utils.cc.)  The code reviewer should then ensure the unit tests capture and correctly test the updated semantics (including edge cases).

    (III) Related to this is that execution of unit tests should be part of the commit process; if unit tests don’t succeeded, the commit fails (unless the committing developer specifically overrides this check——and specifically documents the reason).

    It’s my general view that a code review should require nothing more on the part of the reviewer than an open diff.  There should be no need to set up and run the changes, to create or delete branches locally, etc.  It should be possible to do a full review via GitHub, for example, if one is using that as the source repository.  Anything beyond that stretches what I’d say is under the prevue of a ‘review.’  (Note that this will also increase productivity by allowing a user to avoid having to stash local but uncommitted changes, fetch updates, and so forth, all of which would require unwinding when the review is complete.)

    What we’ve found effective is a simple practice of requesting a code review via GitHub, using inline comments to point out issues or suggestions in the code, and commenting on a commit when signing off.  (We’ve also incorporated git commit -s in the past but this again is something that might be automated via a hook.)

    Copy & paste the code below to embed this comment.
  7. Seems like everyone knows that code review is a valuable tool but a lot of teams struggle with implementing it because of the overhead they feel they just can’t afford right now.  But apart from making your code better, it can also make you, as a dev, better thanks to the entire culture of feedback that the process is connected to. We’ve enumerated a number of reason why should one care to do code reviews (and how we handle it internally): a quick guide to peer code review

    Copy & paste the code below to embed this comment.
  8. Excellent article!

    When I tried code snippet #5 (git log master), however, Git didn’t give me the full log messages of all the commits that were in my feature branch and not in the master branch (instead, I saw all the commits to master, and none of the commits on the feature branch I was on).

    After a bit of digging in the git-log documentation, I tried git log master.. instead (i.e.: I added two dots to the end of the branch name), and I was able to get the results described.

    I’m using Git 2.1.0 on OS/X 10.9.

    Copy & paste the code below to embed this comment.
  9. Hiya,

    I really liked this article, in my last role (Developer in BBC Platform Engineering) the cost of bugs was extremely high so a good review process was essential.

    We did do things slightly differently, each review was normally done in person and we had assigned roles:

    * Presenter - person who wrote the code
    * Time keeper - reviews over 45 mins rarely work out well. Two reviews work better than one long review! Time keeper provided time remaining count downs. Also helps to prevent holy wars and loooong debates.
    * Note taker

    The note taker would take notes on points discussed, but it was up to the developer how they acted on the discussion. We had a rule that only the original developer could make changes to the branch (unless they invited someone to help) as we found this helped people feel better ownership of their work.

    In many ways, the code review represented the moment where the work stopped being owned by the individual, and started being owned (and supported etc) by the team.

    We didn’t have such a refined process for async reviews, but it will be something to look at in the future.

    Thanks again for the article,

    Cheers,

    Jamie + Lion

    Copy & paste the code below to embed this comment.
  10. Very good article on code reviews and advice! I will be referring it for my programming and fundamentals. Thanks

    Copy & paste the code below to embed this comment.
  11. Sorry, commenting is closed on this article.