Tag: Git

How to do High-Bar Code Review Without Being a Jerk

On January 25, 2020

In Careering, iOS Development, Software Development

How to do code review that enforces high standards while avoiding common problems on your team.

Code review is the hardest part of being a developer.

Seriously.

Whereas most of our work is between you and a computer – a machine you just have to operate correctly and indicate clear intentions to – code review is a thing between humans. And like it or not, those humans are squishy, opinionated, emotional creatures. And those traits make code review a challenge.

In principle your goal in a code review is simple: examine another person’s code and make sure their pull request (“PR”) is “good enough” to merge.

But in reality there are a gazillion other things swirling around in your brain.

Urgency – is this a hot fix for a major production outage?
Seniority – am I allowed to decline this senior developer’s PR?
Reaction – will someone yell at me if I decline this PR?
Grudges – will you decline my next PR just because I declined this one?

None of these are related to the code quality, but they are legitimate concerns – you might really rub people the wrong way if you review code unkindly.

So how do you avoid these problems? And how do you set up a team for successful code reviews?

Set the Team Up for Success

Before anyone even writes any code, set clear expectations. Anticipate the obvious problems and just avoid them.

Use a code linter – many arguments in PRs are over style. And thats just not the place for it. You can easily prevent most of these disagreements from making it as far as the pull request by agreeing on a set of linter rules.
Have a code style document – some things aren’t reasonably enforceable by a linter, but are still just conventions. Write these down in a central place and have everyone on the team agree to them. Getting buy-in from your team gives everyone accountability. If the team really did all agree on these rules its much easier to enforce them. All you need to do is link to that page and remind the developer that this was a team decision.
Do design meetings – when starting a big task, consider having a design meeting. This might feel like a waste of time but it’s not. Having the team contribute to the design before the developer starts work will avoid large-scale issues from cropping up when it comes time to review. You don’t want to start a task over because it has a fatal design flaw. And you really don’t want to be forced into accepting a fatally flawed design because you’ve run out of time to go back and start over.
Establish relationships and respect – people are more empathetic and kind when they know each other, and preferably even like each other. Invest in relationships on the team and you’ll avoid some conflicts caused by people just not taking the time to care about each other’s feelings.
Write a Definition of Done – this is a formal, generic list of requirements that apply to all/most tasks. For example maybe “all code must have 70% unit test coverage, relevant documentation must be updated, the CI build must pass, and you must demo your work for your manager.” If any of these aren’t true, the PR should indicate that it is a work in progress, and it shouldn’t be allowed to be merged yet. This makes it clear the submitter is looking for early, broad feedback.

Submitting a Pull Request

Recognize that reviewing code is hard, and make it as easy as possible for your reviewers to give you useful feedback. Remember – you want them to find the problems with your PR. It saves your future self a bunch of headaches.

Write a PR summary – include a brief explanation of the purpose of this PR. Link to the ticket this PR is addressing. If there’s anything odd in your solution, explain it. If there’s a reason you truly couldn’t write unit tests, give it.
Add the right reviewers – don’t add your whole company. Maybe not even your whole team. Recognize that people will take time from their day to help you improve your code when you ask for their review. Just add the people who will give you the most useful feedback – perhaps the subject matter expert of the file you’re working on. Or the person who filed the bug you’re fixing. You will also likely need to add a senior developer who has the authority to accept your work.
Accept the feedback – if someone provides feedback and you can’t articulate a good reason not to, just accept their suggestion. Make their requested change. This may require you to set aside a bit of the pride you have in the great work you’re submitting (and that pride is good!), but it’s not going to hurt anything to accept their suggestion, and you’ll avoid lots of unneeded conflict.

Reviewing a Pull Request

Keep these things in mind when you go to review a PR.

This PR doesn’t need to be perfect. It needs to make the code better. Follow-up PRs are totally ok in many situations, as long as the current PR works correctly, and doesn’t impose an unreasonable maintenance or refactoring cost on others working in the same codebase.
Start with the big picture – pull up the ticket for PR and confirm that this actually does what it’s supposed to. Does it actually work? Does it meet the Definition of Done? Is it clearly missing anything like unit tests? Provide this feedback early so that the submitted has time to rejig the whole PR if needed. There’s no sense in starting with identifying 20 typos (which the submitter may immediately start fixing) only to then suggest they take a completely different approach that will require them to throw all those fixes away anyways.
Next look at the design – how have the classes been broken down? Are the components too big or have too many responsibilities? Do they define clear, testable boundaries? Provide this feedback using the verbiage of established engineering principles. “This design isn’t testable” is much less useful than “Using dependency injection here would allow you to mock out component X in your unit test”.
Next review the “central class(es)” – identify the class that is the guts of the change, and start here. If there are 10 changed files in a PR, most likely only 2-3 of these contain the key logic, and the rest are helper classes or usages of the renamed API, etc. Focus your best effort on the most important classes while you still have the most energy and are providing the best feedback. Then circle back around to the other classes afterwards.
Set a comment limit for yourself – It is difficult for the submitter to address 50 comments on a PR unless they are all trivial nitpicks and spelling errors. It’s also really discouraging to see that huge number of comments – it gives the impression of poor work and may really slow down the developer in future tasks.
Consider in-person reviews for “bad” PRs – if there really are 50 significant flaws in the PR, take the review offline. As someone who can see these flaws, it is your responsibility to help the submitter learn. So sit down and go through the PR together, IRL. Be patient and compassionate, and don’t rush through it or make the submitter feel like they’re taking up your valuable time (even if they are). You are investing in your teammate – it will pay dividends.
Consider in-person reviews for big PRs – sometimes a PR “blows up” and becomes a huge number of changes. In these cases it will likely be impossible (or unreasonably time-consuming) for a reviewer to figure out where to start. So do a group review, and have the submitter briefly present their work. The team can ask a few questions and get their bearings, and either do the full review together, or go back to separate desks and review online now that you have a better sense of what’s going on.
Identify critical vs nitpicks – Be clear about which changes you think are unacceptable, and which are minor suggestions or opinions. These minor items can start with “Nit” as in “Nit: This might be clearer if you renamed this local variable to account instead of a“. Also be clear if a comment is just a question, or if you’re unsure of whether your feedback is valid or not.

If They Always Make the Same “Mistakes”

Over time, you may want to give a teammate the same feedback on many PRs. This is where it becomes easy to be a jerk. Resist. Nobody wants to work with a jerk, and they certainly won’t be receptive to feedback if you treat them poorly.

Remind yourself they are a professional – Nobody writes bad code on purpose. Nobody. Even that lazy sarcastic developer you don’t like for whatever reason doesn’t. So start by trying to help, and take the mindset that you are helping – not criticizing or “fixing” them.
Ask yourself “is this my opinion?” – Just because you disagree with it doesn’t mean it was a mistake. Perhaps they have a valid reason for continuing with the pattern you don’t like, and perhaps that’s ok. You need to let other people win sometimes and recognize what is an opinion. There are many ways to solve all software problems, and we are constantly judging where to draw compromises. You may simply fall on opposite sides of the compromise here – if their code is functional and meets the agreements your team has set out then maybe you should just let it be and tolerate their alternative solution.
Just talk about it – sometimes having a regular old human conversation about the problem will help them understand better why it even is a problem. Have the conversation at a time when they seem fresh and open to feedback, and do it in a place where they won’t feel embarrassed – don’t call them out in front of the whole team, or make them look or feel bad in front of their manager. Choose your first words carefully. “Hey I noticed something I think I can help with – do you have a second?” will be better received than “So you keep making this mistake – we need to talk about it”.
Lead by example – people learn by copying each other. If you demonstrate the kind of thing you wish they were doing, they might just pick it up without you even having to confront them about it. This is also especially important if you do chat about it – make sure that you are on your extra best behaviour and demonstrate your expectations. They’ll be looking for a good example to help solidify the “right way”.

You can do it!

Reviewing code is a super important skill. It helps make your team’s code better, and makes your life better. You need to work and live with this code, so you’re making things easier for yourself by keeping a high bar in your code review. But you also need to be kind to your friends so they stay friends.

Hopefully these tips will help you be a better code reviewer! Good luck!

What is Git? Taking Control of Source Control

By andrew

On January 15, 2020

In Software Development

Let’s say you’re working on a big project – your final paper for a class at school, or a report for your boss. The project is long and complicated, you chip away at it over a period of months, and go through many many revisions of editing.

Then you save your last copy, Big Project Final Version and hand it to your trusted friend to look over. They find 100 more little typos, so you fix those, then Save As: Big Project Final Version 2.

Just to be safe you read over it one more time, and decide you actually want to go back to what you had before in a few places. Hmmm… how exactly was that worded again?

Save As: Big Project Final Version Final.

Save As: Big Project For Real Last One.

Save As: Big Project FINAL.

Then your trusted friends trips over your dog and spills coffee on your computer and you lose everything.

Does any part of this sound familiar?

What is Source Control?

Source control is a solution to this problem. Source control is basically a “saving” system. It is a program you can use to organize your saved copies, easily look back at past versions, keep multiple versions in parallel, and back the whole project up on multiple computers.

Some consumer software also includes systems that do some parts of this job – Microsoft Office and Google Docs have the ability to look at your Version History, for example.

In the programming world, the most commonly used source control system is Git. Other major ones include Subversion and Mercurial. I’ll focus my description on Git, since this is the only system I’ve ever been exposed to.

Git Commits

When you want to save a project in Git, you create a git repository (“repo”). A repo is just a folder with a couple of hidden files in it to help keep track of things.

You can add anything you want to this folder – code, images, copies of other entire programs – whatever! And git can keep track of it.

All you have to do is periodically “commit” your work. This puts a “bookmark” or a “save point” into the history, and allows you to come back to this version later if you want to.

Every time you get your work to a point you want a snapshot of you “make a commit”. This is probably a few times a day, depending on what you’re working on.

Git Branches

The most basic use of Git is as described above. You just keep a linear history of your work, and you can go backwards and forwards in time through these revisions. This is a Git history with only one branch. Customarily, this branch is given the name master, though that’s just a convention – there’s nothing special about that name.

But many projects are more complicated than this, and require keeping multiple, slightly different, versions at the same time. Some of these versions may even be mutually exclusive from each other. Take a plumber’s resumé, for example – they might keep two copies up-to-date – one for getting a job actually doing plumbing, and another version for getting a job teaching plumbing.

These projects have multiple branches. Perhaps one named plumbing, and another branch named teaching.

In Git, we choose a point in the history we want to start from (which is often just the most recent commit), and branch off from there.

Another common use of branches is to use them to work on some new feature or aspect of your project while leaving the master branch unchanged. This keeps master “clean”, and also allows you to have multiple tasks in-progress, without having to finish one before moving on to the next.

Maybe you’re storing the code for a note-taking app in a Git repo. Your next task is to add a “Share” button in your app. So you create a branch called addShareButton. Now when you make a commit, the master version doesn’t change. Instead, only this addShareButton branch has these new commits.

Once you’ve finished working on your new task in this task branch, you can merge it into the master branch, and then your master version has these changes too! You don’t need that task branch anymore, and you can delete it. Again you now only have one master version to keep track of.

Local vs Remote Repos

So far we’ve basically assumed that you’re working alone on this project, but in a work environment that’s rarely the case. You’ll likely have colleagues that work in the same codebase, and want to make changes to the same files as you – often at the same time as you. So how do you do this without tripping all over each other?

Well first lets back up a second to talk about how git is a distributed version control system. This means that everyone can have a copy of the full project history. If you and one colleague are working on a project together, you can both have a copy of the Git repo, and Git will allow you to keep those copies in sync with each other. But neither of those copies is technically “the primary copy” – they are equal. The project has just been distributed between multiple machines.

Now with that said, it is still useful to pretend that one copy of the repo is the main copy – especially when working with teams – and most teams do this.

In addition to everyone having a copy of the repo on their development computers, there is also a copy on a server that is accessible to everyone. This centralizes and organizes your work – everyone can agree to treat that as the “master copy”. When an individual developer finishes a task, they commit their work, and push their branch it to this remote server (usually just called “the remote” or “origin”. Then this branch can be merged into the master branch.

There are a couple key benefits of having your work on the shared remote, and having everyone push to the same centralized repo:

It is easy for everyone to keep up-to-date with everyone else’s work by simply pulling the latest changes from that central repo into their copy on their machine. Without this you would have to separately push your work to each of their machines individually!
Work is less likely to be lost if your machine dies unexpectedly
It is clear when work is done – it is done when it has been merged into the master branch on the remote.

There is also one other key benefit, which is immensely powerful and makes life much easier:

The centralized repo can be managed through GitHub or Bitbucket or other similar software.

Pull requests, code review, feedback sessions

Github has many capabilities. It provides a nice interface for browsing all of the commits and branches in the repo, lets you look at a history of how those changes relate to each other, and is also a directory of millions of projects.

But perhaps the most useful feature it provides is the ability to create Pull Requests.

A pull request is a way of requesting that your branch get merged into the master branch (or any branch). It shows a summary of the changes (which you call the difference (or “diff”) between the branches, and allows your peers to review your work and leave feedback on it before it gets merged.

A reviewer can leave comments/questions, mark their approval, decline your pull request (which temporarily rejects it while you address their feedback), or merge your pull request.

This tool will dramatically improve the average code quality in a repository – partly because people’s feedback will yield useful suggestions, and partly because you try harder when you know someone will be looking over your code with a fine-toothed comb. If you cut corners or do things you know are the lazy way or just plain wrong, someone will probably find those mistakes.

So it holds you accountable for your work, but also is a safeguard around you. Your work is no longer your sole responsibility – you can rely on your team to help spot the mistakes you missed.

Of course this also means that there is more work for you to do – you need to review other people’s code too. And let me tell you – this is often harder than writing the code yourself. You need to try to understand what that code is doing from what little context you can see in the pull request. This is often impossible, or just a waste of time, and a better approach may be to have a real-life conversation with the code’s author, or sit down together to go over the changes.

This can also help ease tensions if you think that the code in the pull request has some fundamental flaw, or you’ve spotted so many errors that writing them all down will overwhelm, discourage, or publicly embarrass the author. These are all bad things to do to a person – and you need to remember to be kind when reviewing someone’s work. They wouldn’t have submitted it for review if they thought it was awful.

Let’s Git This

Git is a powerful tool for managing work shared across a team. It lets you look back in time, see what each other are doing, and make a clear path for how to call something done.

Add Git to your repertoire – it will make you an attractive job candidate, and a better developer. With no need for saving your file versions in separate folders, it will make you more organized.

And some day it might just save you from a world of hurt when you spill coffee into your computer.