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
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?
1 Like
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.
1 Like
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
@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.
thanks for the information Geoff. So git rebase origin/master merges in all changes on origin to the local master branch
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.
until now I didnât do much git fetch operations, because Iâm my own team 
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
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.
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.
3 Likes
We have a Code Review Guide that is mostly about soft skills.
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-urlandfont-urlin Sass files (noturl), 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.
5 Likes
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?
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!
@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.
1 Like
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.
1 Like
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.
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.
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.
1 Like
Whatâs the name of the rules they talk about when discussing not adding more than one instance variable in a controller?