Tag: best practices
Editorial note: I originally wrote this post for the SubMain blog. You can check out the original here, at their site. While you’re there, have a look at CodeIt.Right, which can help you with automated code reviews.
Some weeks ago, I was browsing Twitter when I saw this:
Pair Programming > Code Review— Rafael Ponte (@rponte) November 19, 2017
This prompted a brief discussion between the author and me. He made good arguments, but I left unconvinced that pair programming was the obvious winner.
As someone who’s implemented code review with success and also paired to some extent, I could see how both practices can be valuable. But is one of them clearly better than the other? Are code review and pair programming interchangeable, or are there scenarios in which one clearly shines?
That’s what I’m going to answer today. Let’s dive in.
What I Mean By Code Review
Picture this: you’re a young programmer on your first development job. After you finish your first assignment, they call you to a meeting room; there you find your tech lead and three senior developers. There’s also a large monitor displaying your code. Time for review!
For two-and-a-half excruciating hours, they scrutinize your work while you sweat profusely. From criticizing your high-level design decisions to nitpicking over the most trivial stylistic preferences, nothing gets a free pass.
Is this the scene your mind conjures when you see the words “code review”?
Good news, then. That’s not the kind of review I’m talking about. Instead, think of a very informal and lightweight process. You submit your code for review by creating a pull request or maybe using some embedded functionality on your IDE.
After touching briefly on the what and how of a code review, let’s get to the why. Why should your team adopt this practice? What are the benefits?
The first reason is, not surprisingly, to catch bugs. I’d bet you’re familiar with that old piece of software wisdom that says that the later a defect is found, the higher the cost of fixing it. Then why not use a relatively cheap process that can catch up to 60% of defects?
Here’s another reason your company should perform code reviews: to improve the readability of the code. Trying to read and understand some new piece of code frequently leads to the discovery of issues such as
- Poor naming.
- Bad indentation.
- Instances of cargo cult programming.
- Non-idiomatic code.
The reviewer could also spot overlooked corner cases or help the author assess how performative their code is.
And here’s one last reason. A well-done code review can spread knowledge throughout the team. This improves the product’s quality by destroying knowledge silos.
For the most part, their complaint is time. When you submit some code for review, you’ll have to wait until the reviewer is done.
What should you do while waiting? Ideally, your team should break up the available work in small, discrete units so it can work on these pieces somewhat independently. That’s not always feasible, though. And by the way, the constant switching of tasks might be detrimental to the developer’s focus and productivity.
Let’s say Bob spends x hours implementing a feature. Then Alice reviews his work and says his implementation is completely wrong, and he needs to start from scratch. Those x development hours were just thrown away.
Finally, people sometimes will waste ridiculous amounts of time arguing on pointless stylistic details, such as the position of braces or whether or not to include an underscore before a private field’s name. These debates can unfortunately escalate to levels that turn the workplace toxic.
Pair Programming: Not Just a Super Code Review
Pair programming is a technique in which two developers collaborate on the code together, sitting at one workstation.
They periodically take turns in two roles. The driver writes the code, thinking out loud in order to explain their design decisions and thought process. The navigator observes the driver’s work, giving real-time feedback and advice.
So, is it just “code review on steroids”, as put by Jeff Atwood?
Maybe not. One of the basic principles of agile methodologies is the shorter your feedback loop, the better you are. You can see how getting someone to review your code sooner rather than later works in harmony with the principles that make agile so successful.
Well, it should be no surprise that many benefits of pair programming are also benefits of code reviews, such as fewer bugs, improvement in code readability, and knowledge dispersion throughout the team.
Pair programming may provide exclusive benefits as well, such as:
- Higher focus. The presence of a peer may exert some pressure to stay motivated to solve the task at hand.
- Shorter feedback cycle. Since your pair is reviewing your code in real time, there’s a lot less risk of wasting time due to delayed feedback.
- Increase in usage of other good engineering practices. As suggested in a study done at North Carolina State University, teams using pair programming tend to increasingly use development practices such as unit testing, continuous integration, and establishing coding standards.
As in the case of the code review, pair programming is far from being a universally accepted practice. While many developers love it, others don’t have such happy stories to tell. So, what are some of the most cited problems with pairing?
Let’s start with one common complaint: pair programming can be exhausting. In fact, many claim that pairing is more effective when used for shorter blocks of time—from one and a half to two and a half hours.
Obviously, an odd number of team members doesn’t work well for pair programming. But a shifting number of available personnel is inevitable.
Next on our list of problems is the fact that paring isn’t very remote friendly. You can better imagine the issue after hearing what Daniel Kaplan, who wrote “What It’s Like to Pair for a Year,” had to say about pairing:
These scheduling interruptions happen, but on a typical day we avoid them by having the pairs show up at the same time (for breakfast and standup), go to lunch at the same time, and leave at the same time. This maximizes the time the pairs are pairing.
So, pairing requires synchronicity, which might make it a non-option for remote teams (or even co-located teams with flexible hours).
Some people argue that pairing can undermine creativity and prevent experimentation. While pairing, it’d be rude to waste your pair’s time trying some experimental approach that might ultimately end up not working. So, the safest possible design tends to always prevail, even if it’s not the best possible one.
Finally, pairing doesn’t really provide one of the key benefits of the after-the-fact code review: a person with zero context analyzing the code. The two developers share a context from the beginning of the session, and the effect this has cannot be underestimated. People tend to overvalue their contributions and get emotionally attached to what they create; that’s why it’s so important to get a second person that doesn’t have this attachment and is thus able to provide a clearer judgment.
Code Review vs Pair Programming: The Verdict?
I’ve reached the conclusion that, although code review and pair programming seem equivalent, they’re really not. There is some overlap, but each practice also presents some unique benefits and challenges.
There’s no getting around that pair programming, despite its benefits, requires an even number of people, working at the same time. If your team consists of developers living across several time zones (or even a co-located team with extremely flexible hours), then it’s a no-brainer: code review to the rescue.
If your team doesn’t fit the above description, then give pair programming a try. As long as you work hard to accommodate and have empathy for different kinds of personalities and don’t make it mandatory, it can be beneficial to your team.
Finally, there’s nothing stopping you from using both. You could adopt pair programming as the default MO and leave soloing and code review to fill in the gaps where pair programming doesn’t quite fit so well.
No Matter What You Do, Embrace Automation
Imagine you write for a publication, such as a magazine. After you have a draft, you submit it for review. Would it make sense for the editor to spend all their time looking for spelling errors? Of course not! We have automated spell-check for that, freeing the editor to look for more high-level problems, such as poorly chosen words, lack of cohesion, inappropriate tone, and all those things your English teacher kept nagging you about in high school.
With software, things should not be so different. By using an automated code review tool, you can eliminate a lot of the bickering that often occurs in code reviews or pair programming sessions. There will be no arguing about naming and formatting conventions, the position of brackets, and others pointless trivia.
You can also employ a static analysis tool to warn you about potential bugs and opportunities for refactoring. That way, the reviewer/navigator is free to focus on the high-level stuff that requires human creativity, intelligence, and empathy.
Trust and Respect
While researching for this post, one theme kept reappearing: that code review emerges from a lack of trust in our developers or that pair programming infantilizes them.
I couldn’t disagree more.
Precisely because we respect our developers—and our clients—we should employ techniques and tools at our disposal to improve the quality of our work.
It’s not about lack of trust. It’s about recognizing that programming is hard and sometimes, just one brain isn’t up to the task.read more...
Editorial note: I originally wrote this post for the NDepend blog. You can check out the original here, at their site. While you’re there, download NDepend and give it a try.
I first learned about cargo cult programming a few years ago. I remember thinking back then, “What a strange name for a programming-related concept.”
If you share my past self’s astonishment, then today’s post is for you!
First, you’ll see what cargo cult programming is and why you should care. Then, we’re going to look at some practical examples, using the C# language. Finally, we’ll close with advice about what you can do, as a developer, to avoid falling into this trap.
Cargo Cult Programming: Doing Stuff Just Because
According to Wikipedia, “Cargo cult programming is a style of computer programming characterized by the ritual inclusion of code or program structures that serve no real purpose.”
In other words, it’s when a developer writes code without really understanding it. The developer may use a very trial-and-error approach—maybe copy and paste some code from somewhere else and then tweak it and test it until works, or sort of works. Then the developer will stop tweaking the code, for fear it will stop working. In the process, maybe they leave some lines of code that don’t do anything.
Or maybe they tried to use an idiom they picked up from another developer while failing to understand that the contexts are different and it’s useless in the current situation.
Finally, it might just be lack of education: maybe the developer has a poor mental model of how the tools they’re using really work.
Why is Cargo Cult Programming a Problem?
As Eric Lippert puts it, cargo cult programmers struggle to make meaningful changes to a program and end up using a trial-and-error approach since they don’t understand the inner workings of the code they’re about to change.
This is not so different from what the Pragmatic Bookshelf calls “programming by coincidence”:
Fred doesn’t know why the code is failing because he didn’t know why it worked in the first place. It seemed to work, given the limited “testing” that Fred did, but that was just a coincidence.
That single sentence pretty much sums it up for me: if you don’t know how or why your code works, neither will you understand what happened when it no longer works.
Origin of the Term
Although practices that are considered cargo cult today have been recorded as early as the late 19th century, the term itself dates from 1945, when it was first used to describe practices that emerged during and after World War II between Melanesian islanders.
These islanders would mimic the soldiers’ behavior, such as dressing up as flight controllers and waving sticks, hoping that airplanes would descend from the skies with a lot of cargo.
Since then, the term cargo cult has been used in a variety of contexts to mean to imitate form without content—to perfectly copy the superficial elements while failing to understand the deeper meanings and workings of whatever one’s trying to emulate.
Talk is Cheap; Show Me the Code!
Enough with the history lesson. Time to see some code! I’m going to show you five examples of cargo cult programming in the C# language.
Checking a Non-Nullable Value Type for Null
This one is a pet peeve of mine since I see it a lot in production code. It goes like this:
Here we have a developer that probably doesn’t grok the difference between value and reference types. It would be completely forgivable, in the case of a junior developer, except for the fact that the compiler warns you about that.
You could argue that I’m nitpicking. After all, the code will run fine in spite of this. In fact, the check won’t even be included in the resulting IL, as you can see from this print of a decompiling tool:
You can see in this code snippet that the compiler has optimized the null check out.
There are plenty of worse problems, granted. Yes, the application won’t crash because of this. So what’s the big deal?
Well, for starters, I’d be worried about a development shop where the sole quality measure was “it runs without crashing.” But the real problem is that this type of code shows a lack of understanding of some fundamental characteristics of the language and platform that can bite you in the future.
Unnecessary Use of ToList() in LINQ to Object Queries
Like the previous one, this is something I routinely see in production code. Consider the code below:
The problem we have here is that these calls to
ToList() are completely unnecessary (except maybe the last one, if you really needed the result to be a
List and not only an
In my experience, this happens when the developer doesn’t understand the nature of LINQ; they erroneously think that the LINQ methods belong to the concrete type
List<T> instead of being extension methods that can be used with any
ToList() several times like this, the developer creates several new lists, which can be detrimental to the performance of the application.
You could rewrite the code above like this:
Consider the following line:
Here we have not only one but two unnecessary conversions. First, the developer creates a new string and then parses it to
DateTime when a simple cast would have sufficed:
This example assumes that the underlying database type is some specific type for dealing with dates (for instance,
datetime in SQL Server). Of course, if you were using an inadequate type (such as
varchar) then this would be a problem of its own.
Also known as Pokémon syndrome (“Gotta catch ’em all!”), the anti-pattern here is to add a try-catch block to every single line that could possibly throw an exception.
Bonus points if the code is attempting to catch
System.Exception instead of a more specific exception, thus blurring the distinction between expected and unexpected errors.
More bonus points if the catch block doesn’t contain any code at all!
The general advice here is this: never catch unless you have a very specific reason for doing so. Otherwise, just the let the exception bubble up until it’s dealt with by the top-level exception handler.
If this advice seems vague (“How would I know if I have the right reason for catching an exception?”), that’s because it is vague. It’s beyond the scope of this post to go deeper into this matter, but Eric Lippert’s excellent article called “Vexing Exceptions” will greatly improve your understanding of exception handling.
Using StringBuilder Everywhere
It’s the stuff of superhero movies: after reading somewhere that concatenating strings by using the ‘+’ operator is incredibly inefficient, the well-meaning developer takes upon themselves the Herculean task of updating every single concatenation in the codebase to
The reasoning for this is, of course, that
System.String is immutable. So every time you “modify” a string, you’re in fact creating a new instance in memory, which can hurt performance pretty badly.
Well, guess what? The compiler is pretty smart. Let’s say you have the following line:
This, in fact, gets translated to
The fast rule of thumb is it’s fine to use the simple concatenation when you know the number of strings to append in compile time. Otherwise, a
StringBuilder probably makes more sense.
Of course, some scenarios aren’t that clear-cut. The only advice worth giving here is to do your homework. When in doubt, research and benchmark to your heart’s content.
I’ll leave you with more sound advice from Eric Lippert:
Unnecessary code changes are expensive and dangerous; don’t make performance-based changes unless you’ve identified a performance problem.
Is There a Remedy?
I’d say it’s fair to assume that more inexperienced developers are more prone to commit mistakes due to cargo cult programming. But no developer is really immune to it, independent of their knowledge or experience.
We’re only human after all. Tiredness, deadlines, cognitive biases, and (to be really honest) the eventual laziness can turn even the best developer into a cargo cult programmer.
Unfortunately, there’s no 100% guaranteed way of preventing this from happening. Yet there are some measures you could take to, at least, decrease the odds.
Let’s take a look at some of them.
Use Code Review/Pair Programming
The first measure you could take to avoid cargo cult programming is to simply get a second pair of eyes on your code. The benefits of having a second person reviewing each line of code before it goes to production can’t be overstated. And while code review and pair programming aren’t perfect equivalents, both of these practices will bring you this benefit.
Always Test Your Hypothesis
Write unit tests (and other types of tests as well). Monitor your application in production. If something doesn’t perform well, benchmark the heck out of it. Don’t just assume things. Testing your hypothesis can bring valuable insights and save you when your intuition gets it wrong.
Read Other People’s Code
Reading other people’s code is a great way to learn. It’s a perfect tool to compare your own ideas and assumptions against what other developers are doing, exposing you to novel concepts that can force you to gain a deeper understanding of the issues at hand.
In the era of GitHub, there isn’t much of an excuse for not doing that.
Learn From Your Tools
There are currently a plethora of tools that can help your team improve the quality of their code. Here’s the thing, though: you shouldn’t just use these tools. You should also learn from them. If you use NDepend, read about its rules. Try and understand the rationale behind them. What are the principles and best practices that guided its authors when coming up with the rules?
The same goes for other types of tools—and even the warnings the compiler gives you.
Computer Science, Not Computer Superstition
Even though no one is immune to cargo cult programming, we should strive to overcome it. There’s hard-earned industry wisdom at our disposal, slowly generated over more than seven decades. Let’s use it. Let’s understand our tools and our craft and write better software.read more...
Spoiler Alert: You should avoid most of them.read more...
Some people believe private methods should be avoided. Are they right?read more...
There are only two hard things in Computer Science: cache invalidation and naming things.
Do you want to write great code? Clean, understandable, human-readable code? Well, there are several skills you need to acquire. But I’d say #1 on the list is “Picking Good Names”.read more...