Friday, May 5, 2017

The importance of code reviews

In my discussions with development teams on how they can optimize their development processes and deliver high quality software, a topic that keeps coming up is the importance of doing code reviews. Many developers I meet with have a relaxed opinion on doing code reviews: they appreciate the quality assurance involved in someone else reviewing their work, but doing code reviews for others is often de-prioritized as the work load grows and the deadline is close.

There are three main reasons for why you should do code reviews:
  1. Code reviews reduce risk
  2. Code reviews minimize the amount of technical debt introduced into a code base
  3. Code reviews increase team collaboration
I'd like to go into detail on these three reasons to explain why you should increase your efforts on doing code reviews, for the sake of yourself and the rest of your team.



#1 Code reviews reduce risk

If you think about the amount of risk involved in creating a piece of software, it's a wonder we're brave enough to work as developers. One mistake and you could be tomorrows media headline (just ask the developers at Knight Capital).

Doing code reviews reduces the risk of a developer introducing security and privacy vulnerabilities to the code base, simply because having a second pair of eyes questioning what is committed will act as an extra verification. We've all met that one dev who tries to implement their own encryption method, or who outputs "This e-mail address is already in use" on a registration page, or who believes it's best to store all user data just in case we need it. With a code review, features that are implemented in poor judgement can be stopped or improved before making it to production. When it comes to matters of security, not all developers can be (or should be) security experts. How about identifying high-risk parts of your application and having one of your security experts do mandatory code reviews on those parts before the change is allowed to make it any further than a feature branch?

So far, I have only mentioned two types of risks: Security and privacy. The third type of risk is one of my favorites, called the "bus factor".

"The bus factor is a measurement of the risk resulting from information and capabilities not being shared among team members, from the phrase "in case they get hit by a bus". 

(Source: https://en.wikipedia.org/wiki/Bus_factor)

For those who have read "The Phoenix Project", you will remember Brent Geller who is called upon by everyone, at all times. He is the one guy who knows everything and who would leave the organization clueless if he were to be hit by a bus. A guy like Brent, together with the rest of his company, would benefit from a development process where other developers were forced to understand what he was doing and why. And remember how our hero Bill manages to reduce the bus factor in PartsUnlimited? He refuses to let Brent fix anything without another developer being part of the learning experience. He basically introduces pair programming and code reviews. Smart guy, huh?

(If you have no idea who or what I am talking about right now, stop reading this blog post and go read "The Phoenix Project" instead.)

#2 Code reviews minimize the amount of technical debt introduced into a code base

Technical debt in a code base is inevitable. If I didn't know better, I would believe that the second law of thermodynamics, which states that the total entropy of an isolated system can only increase over time, also applied to code. Sounds like something developers can relate to, right?

Humans are simple creatures. If we know someone is watching, we are more sustainable to doing the right thing instead of making shortcuts. If you know you are the only one looking at a piece of code, doing a quick and dirty fix, thinking "I'll clean this up later" will feel a lot easier than if you know that you have a colleague who will be looking at your code before it's allowed into your master branch. So the simple notion of knowing that someone will review your work will increase the overall quality of the code that you are writing.

There are several other reasons for technical debt than dirty quick fixes, of course. As your application grows over time, the architecture will become more complex, it becomes more difficult to avoid duplication of business logic and developers tend to want to rewrite someone elses code rather than refactor and improve it. Looking beyond the manual aspect of one developer reviewing another developers code, code reviews can include automated verifications to slow down the process of software rot. For example, you can set up an automated build that runs all unit tests and fails the build if any new tests are failing because of the changes you've made. That way, you are notified that you need to improve your efforts even before one of your colleagues sit down to review your code. Another example is running static code analysis, picking up on any credentials, keys or sensitive data that has been committed, automatically failing your pull requests.

So far, I've mentioned many concepts, such as feature branches, pull requests and the master branch, that are part of the GitHub workflow. If you feel a bit shaky on what these concepts are, I recommend this great visual tutorial on Understanding the GitHub Flow. If you're interested in seeing how Microsoft tools support this, keep on reading.

#3 Code reviews increase team collaboration

My favorite argument for why code reviews are important is a simple one: Ownership. Developers who feel like they own their code and who feel like they have a resposibility for their code base, will take better care of it. It works in the same way as renting vs buying a house. If you own it, you're more likely to invest in maintenance than if you rent the place.

Ask yourself this: Have you ever had a bug reported, where the developer receiving the bug report said: "Uh, but this is Steve's code, I don't know how this works"? This it what happens when developers don't view their software development as a team effort, simply because they don't work as a team. They have separated their development and their responsibilities into small bits and pieces of "This is mine and this is yours". There is no ours. In a developement team who actively does code reviews, it will be easier for one developer to continue the work of another, or to fix a bug introduced by someone else because they have seen the code before. Hey, they've even approved the code before, meaning they should understand it pretty well.

Last, but not least: Code reviews function as a way of upskilling. All development teams have members with different qualifications and seniority. Having a senior developer review the code of a junior developer can act as a verification of quality, while having a junior developer review the code of a senior developer will act as a learning experience. It will broaden their understanding of the archtectural descisions made and give them insight into patterns and practices they might not be familiar with themselves. Upskilling through code reviews will give you "T-shaped" developers, meaning your developers will have a broad skillset and the ability to collaborate across diciplines, while maintaining their targeted expertise in one area.

What now?

One of the common misconceptions of code reviews and the argument I hear most often for why developers don't do code reviews is that they don't have the time. Many tech leads are not able to see beyond the initial upskilling period that comes with any change introduced into your development process, and they are therefore blind for the true benefit of doing code reviews. Setting up a development process where developers are forced to review each others code will in the long run speed up the development, make you less prone to efficiency blockers such as vacations, unscheduled time off, developers changing jobs and (let's hope this one doesn't happen) someone being hit by a bus.

Code reviews in Visual Studio Team Services

In Visual Studio Team Services (VSTS), you can define branch policies for the branches in your Git repository:

 
In the example above, I have enabled branch policies for my master branch, requiring all changes to be submitted through pull requests. The creation of a pull request will queue a new build (based on my "dotnetcore CI HOL" build definition) and if the build fails, the pull request will be blocked. I am also checking that all pull requests have been associated with a work item and that all comments have to be resolved before the pull request is merged. At the bottom, I have declared that minimum 2 reviewers must approve my changes before I can merge, and I cannot approve my own changes.

Now, let's take a look at the code review experience that comes with a pull request in VSTS:


Here you can see the same policies (and some additional ones), the work items linked to this pull request and the reviewers.

As a reviewer, I am able to comment on the changes and approve them. I can also auto-complete the pull request, meaning that as soon as all the necessary reviewers have approved the pull request and all policies are green, the pull request will automatically be merged. This way you don't have to return to the pull request after a long running build has completed, it's all about improving the efficiency of the developers.

<sales-pitch>
If you would like to try this out, you can create a free VSTS account for up to 5 users. If your team is larger than that, anyone who has a Visual Studio license has access without additional costs.
</sales-pitch>

My hope is that more development teams will optimize their development efforts by taking advantage of doing code reviews. Feel free to share your thoughts!

Friday, February 10, 2017

From developer to seller: An identity crisis

After I started working at Microsoft 10 months ago, the amount of blogging I did naturally decreased as I wanted to focus all of my attention on getting settled into my new job. Everyone says that it takes a year to get fully settled at Microsoft, and although I didn't believe them when I joined, I can confirm that this is true. After slowing down on the blogging and social media activities for a period of time, I came to a complete stop. I blamed it on the same reason as before: I'm focusing on my new job.

Today I realized that that's not true. I didn't stop because I wanted to focus on other activities, I stopped because I lost my motivation. And I lost my motivation because I've had a bit of an identity crisis.

If you had asked me 5 years ago if I would ever consider doing something else than work as a developer, I would have laughed and answered: "NEVER!" Software development is what I love, nothing excites me and motivates me more than seeing innovation in the form of technology.

After a couple of years though, I started to feel the need of being part of something bigger, something more powerful, something that can make a great impact on the lives of both developers and those who don't write code. Someone whose mission is to empower every person and every organization on the planet to achieve more. Microsoft.

Before I go on, I will have to explain a bit more of what I do at Microsoft. You can find the long version here (http://www.karolikl.com/2016/04/my-first-two-weeks-at-microsoft.html), but I'll give you a quick summary. I'm in charge of our developer tool offerings in Norway: Visual Studio, Team Foundation Server and Visual Studio Team Services. I sell these tools and I help developers understand how they can use these tools to improve their software development process.

I'm no longer a professional developer myself. Sure, I still write code, but that code is for demos or proof of concepts. I no longer write production code on a daily basis. And here's the catch: The transition from being a developer to a seller has been more challenging than I thought, and I haven't been able to accept my new identity as a non-developer until today. It took me 10 months and 10 days to accept that I am, in fact, a seller. I'm not a developer. I'm a seller.

As I haven't been able to accept my new identity, I haven't been able to blog. I'm not sure I'll be able to explain why, but it has to do with credibility. If you've read my technical blog posts from earlier you see that my only intention was to share great tools, experiences and practices with other developers. I never tried to sell, I was never compensated for writing about those tools. I was a huge advocate for both EPiServer and Octopus Deploy (and I still am), but I only blogged about them because I wanted to share the awesome experiences I had, hoping that other developers would somehow find it useful.

Blogging as a seller feels different. Everyone knows that I am compensated based on the adoption of Visual Studio, and during my little identity crisis I believed that this meant I no longer had my credibility when writing about these tools. I was scared that you as a reader would think: "Does she really think these things are cool or is she just trying to make us buy it?"

I realize now that this is something I don't need to worry about, simply because I would never blog or speak about something I don't believe in myself. It sounds like a simple conclusion to a simple process, but believe me: There's nothing simple about it. This is the result of a tranformation that has taken me over 10 months to accept.

Today, I am back. My motivation to blog and share is back. And I can finally say the words without feeling nauseous: I'm a seller.

</ identity-crisis>

Thanks to those of you at S4 who made me come to this realization, you probably don't even know that you helped me.

Have you been through a similar transition yourself? I'd love to hear about it!