← Back to Upcase

Tips For Code Review


(Upcase ) #1
Ben and Joe review several pull requests against the Learn database, pointing out techniques and patterns for having a constructive and efficient code review. Learn how the thoughtbot protocol leads to cleaner and faster reviews. Follow along as w...
This is a companion discussion topic for the original entry at https://thoughtbot.com/upcase/videos/tips-for-code-review

(Will Greenway) #2

Great video, as always! As I am starting to shift to this type of code review model its great to see how others are doing it in the real world.

Early in the video when Joe mentions that pull request messages should essentially be in the git commit message, he says that before opening a pull request they “try to get everything revised into one small, readable, commit”. Does this mean that after working on a feature branch all commits are squashed into one before requesting a merge?

In the case that the answer to the above is “yes”, I’m curious how others feel about that practice. Is there any sense that some meaningful meta-information about the feature is lost by consolidating commits? You get to see the overall change, but if one line item is “refactored this method name”, without the sole commit that accomplishes that refactoring, the ability to easily detect what just that part of the feature affected is lost.

On a related git process note: if multiple people are working on a feature branch and it exists on origin, before opening a pull request do you rebase the feature onto master and force push?


(Derek Prior) #3

In my case, no. I’m perfectly happy leaving as many commits as I think are valuable to have in the history. Sometimes that’s two commits, sometimes that’s three (it’s seldom if ever more than that), but it usually does happen to be 1. The number of commits I feel are appropriate is not necessarily related to the size of the feature either. My goal is to have history tell the right story.


(Acandael Acandael) #4

Great video. I also went to the Git part of Thoughtbot Guides Protocol.

It says:

Rebase frequently to incorporate upstream changes.

git fetch origin
git rebase origin/master

I’m not fully getting this, if you do git fetch origin, then why you still need to do git rebase origin/master ?

greetings,

Anthony


(Geoff Harcourt) #5

@acandael_acanda, git fetch pulls down all the references (so knowledge of all the commits and branches) that are on origin, but doesn’t perform any merging.

http://git-scm.com/docs/git-fetch


(Acandael Acandael) #6

thanks for the information Geoff. So git rebase origin/master merges in all changes on origin to the local master branch


(Geoff Harcourt) #7

If I understand this correctly, you fetch to make sure that your machine has all the references that are stored on origin, and then the rebase happens against your machine’s local copy of those references.


(Acandael Acandael) #8

until now I didn’t do much git fetch operations, because I’m my own team :smile:

But in the near future I would love to work together with my collegue (a former Java programmer) on Rails projects.

So for now most of the time I just do git push operations to my remote repositories and heroku production repository.

But if I get it correctly, when working in a team, and working on a feature branch, it’s best to do a git fetch and the a git rebase origin master, right?

greetings,

Anthony


(Geoff Harcourt) #9

I worked completely alone for a few years before finally doing more frequent collaboration with other engineers. It’s really fun, but it underscores how important it is to follow good, disciplined git behavior when you work with others. The flow Joe describes in the video (branching, using the PR system, not force-pushing until your final commit on the feature branch, then merging to master) is a good guide for how to build software alone or in a team. (You can skip the PR system part, but it’s actually a nice way to do review yourself and to document major efforts.)

Even if you’re working alone, a good reason to use feature branches is that if your work is in-progress and not ready for deployment, you can always stash it or make a WIP commit and then go back to master and bring out a new branch for your bug fix. If you do all your work on master and you’re in the middle of a large feature, you lose that agility in the face of unexpected emergencies.


(Dan Croak) #10

One thing not covered in the screencast but that I think is a great point is “priority of reviewing a teammate’s code versus other tasks I need to do during my workday.”

Sara Haider on the Giant Robots podcast mentioned the idea of a code review as Service Level Agreement (SLA): there should be the shortest possible mean time to respond to teammate’s request.

The reason is that when a designer or engineer needs a review, that is something that is closer to being ready for users, which is (arguably) more important than the reviewer’s work in progress.


(Dan Croak) #11

We have a Code Review Guide that is mostly about soft skills.


(Dan Croak) #12

It can be hard for me as someone who is mostly a web developer to code review a designers’ or mobile developer’s pull requests. Here are some areas where I feel I can add value:

  • Looking for hygienic things like detailed commit messages, a URL to the Trello card (or other project management system’s ticket).
  • Looking for asset pipeline things like using image-url and font-url in Sass files (not url), so the asset pipeline will re-write the correct paths to assets in production.
  • Looking at a failing build to provide suggestions on how to fix it.

Perhaps the most important thing we can do when we’re collaborating across skills/roles, though, is anything product-level:

  • Asking the for an animated gif screencast using a free tool like LICEcap.
  • Asking for a URL to a deployed feature branch, or ngrok URL that points to the commit author’s local machine running the branch.
  • Asking for acceptance criteria on the GitHub pull request message if I don’t see it on the PR or the linked project management ticket.
  • Checking out the branch locally, running the app on my machine, and observing the user experience.

This is all similar to the idea that Joe mentioned in the screencast, of making sure as a reviewer, you understand why a change needs to exist before really reviewing the code. That affects everything else about how you review the change. Is it a change that only needs to live for two days during some particular campaign? I probably won’t ask for super-pure OO and lots of specs. Does the change affect new pricing? I might be super-careful about making sure there’s no logical mistakes but also try to imagine how our users might psychologically react to copy text.


(Geoff Petrie) #13

This was another great episode and a very good discussion! Thank you for the Weekly Iterations. They’ve been terrific!

The question I have regarding code review is what advice people have for people who have been on receiving side of a poorly handled review? I’m not speaking of people having constructive criticism of your code, I can’t get enough of that, but of reviews that are poor in tone, or avoid higher level conversation in lieu of discussions of using things like specific hash syntax.

In other words, as the reviewed how can you make the conversation more valuable if the reviewer doesn’t appear to know a good way to do so themselves?


(Mark Thomas) #14

One thing I’d be curious about is doing ‘self’ code review; I know it’s not the ideal way to go about it, but in the team I’m on I’m the only developer and so I don’t have others to review my code; does anyone have any thoughts/tips/insights? Thanks!


(Dan Croak) #15

@Mark_Thomas Not the same as a human review but tools like http://travisci.com (test suite), http://codeclimate.com (code quality), and http://houndci.com (style) can be helpful for automatically catching a large number of issues.


(Geoff Harcourt) #16

Echoing @croaky, those tools are helpful in keeping you honest when you work alone.

I found that CodeClimate helped me identify classes I had allowed to grow too much before they got completely out of control.


(Mark Thomas) #17

Cool—thanks!! I’ve been using CI and linters for a while but haven’t really gotten into using Code Climate; is it really pretty worthwhile? I’ve heard good things about it but am still a bit skeptical. I suppose it can’t hurt to use though, even if it might not replace real code review.


(Geoff Harcourt) #18

You can get some of what CodeClimate does by running Flog, Flay, Reek, and Brakeman against your app, but I find their presentation of problems and the automation linked to their analysis is really nice.


(Joel Oliveira) #19

Hope it’s not out of turn to mention this but - For a little while I’ve been working on something to help make code reviews and pull requests (from real people) more accessible to beginners, freelancers, hobbyist developers, etc. If anyone’s interested in a beta (when that’s ready), feel free to get in touch. Either here or twitter (@jayroh) or whatever. I can give the run-down for what it is and would love feedback for those interested.


(Daniel Infante) #20

What’s the name of the rules they talk about when discussing not adding more than one instance variable in a controller?