Readability Above All Else in Code Reviews

In my last post, I made a case that code reviews cannot be relied upon as the main activity to achieve defect prevention, correctness, robustness, usability, or learning. Even so, these goals are worth pursuing in code reviews, though other practices such as unit testing and QA should be the primary driver to such goals. Aside from those goals, code reviews can indeed be the primary driver of other areas of improvement for a software team. It is arguable (comments welcome), but I say above all else code reviews routinely and consistently improve code readability. Highly readable code might not be the first measure of software quality or of team competency that you'd list, but it should be. Code is typically written once and revised a few times, but read dozens of times. Programs are for humans to read and only incidentally for computers to execute. I or any competent developer can deal with any code that we can understand, and the quicker we can understand it, the faster it will be to troubleshoot, maintain, and enhance. Readable code gives us more than a fighting chance. Too often, when teams are under the gun and avoiding spending any time or effort into code review, we get indecipherable, weird, arcane, and confusing code, and this incurs troubleshooting, maintenance, and enhancement costs to go way up in the future, and in unpredictable ways.

The most important imperative in generating highly-readable code, I say, is: write your code so that the reader can immediately and intuitively discern what it actually does. Don't make me think too hard. Make the code obvious. As a reviewer, you can make this your top priority when recommending changes to the code under review. A reviewer ought to first point out where she or he doesn't understand what the code is doing and try to collaborate with the author to make it obvious. As an author, when a reviewer doesn't immediately grok what you wrote, your first thought shouldn't be "Okay, let me explain it to him" and certainly not "I should probably add some comments that explain this". Instead, think "If he didn't get it, others won't get it either. What code should change to make this more obvious?"

More often, the reviewer already understands what the code is doing but quickly thinks of a better way to express it--"better" here being synonymous with "more readable". Code review is the only practice I know of that presses code into a more readable state routinely and reliably. These can often be the quick wins: the reviewer says "try this instead", and very often the author can quickly agree and integrate such a change. As an author, I am very apt to take my coworker's suggestions, certainly when I am ambivalent and even when I mildly disagree. The rapport and mutuality gained from adopting a reviewer's recommendation in code review might be more valuable than the change itself. And several times I have looked back on such changes after several months and been convinced in the end--turns out my teammates are really quite good at this programming stuff!

We should all write our code so that the mapping from the code itself to our intent is as simple and as straightforward as possible. For example, if I intend for a method to calculate and then save, naming it CalculateAndSave is better than naming it Save. In a code review, pointing out "this is doing more than saving" will at least result in a renaming of the method. Then, that name then might lead us to question if the calculation should be done in a separate method for better separation of concerns. Conversely, if I named a method ValidateAndSave but there is no validation code in the method at all, my teammate would immediately point out that I intended to validate but never actually did so. Pointing out the differences between apparent intent (or mutually-understood requirements) and actual code is a top benefit of doing code reviews.

So what makes code highly readable?  Sometimes "it depends" (of course). Sometimes, shorter code is more readable (e.g. omitting redundancies), and sometimes longer code is more readable (such as declaring a semantically meaningful temporary reference). Sometimes clever code is more readable (i.e. when your cleverness came up with the apt statement; please not when cleverness is needed to understand it); sometimes "dumb" code is more readable (especially when familiar and unastonishing). Sometimes more abstraction (more layers) is more readable; other times, collapsing layers makes it more readable. When these questions come up in code, the important thing is that the team is deciding together which way to go in each case. If the team is skilled and genuinely cares, I trust them to make a decision, even if I disagree with that decision. The difference between my idea of good code and my team's idea is usually tiny compared to code that isn't reviewed for readability at all.

However, there are some good readable-code rules-of-thumb that are almost always true. Simple beats complex. Boring beats exciting. Code written as others expect beats astonishing code. Conventional code beats innovative code. Pattern-following code beats trailblazing code. Linear code beats nested code, and shallower nesting beats deeper nesting. More short methods are better than fewer long methods. I think we all appreciate simple, boring, as-expected, conventional, patterned, linear, shorter code--so as a code reviewer, be eager to give your recommendations to make it such.

And there is one more rule-of-thumb that I hold to be the most important: refactored code beats original code every time. Code can almost always be made more readable by being revised, especially when there is no pressure to change its behavior. This is exactly where team peer code review shines: It allows for code to be refactored mere minutes or hours from when it was originally written, with zero added cost of extra testing, when you are most familiar with both the desired and actual behavior of the code in question, when such refactoring is typically least costly and most welcome by all concerned parties. It is the biggest no-brainer when it comes to code review: seize the opportunity to refactor your code to be more readable, and leverage code reviews to spur changes for readability.

Of course, even if all that is agreed among team member, opinions will still differ as to what is easiest to read. One thinks nested ternary conditionals are easier than using elseif; their teammates think they're nuts. Some like indenting via tabs, some like spaces. When these differences are aired in a team peer code review, a quick resolution is likely. If the teams uphold the value of others' work and want to come to a consensus, a disagreement in a code review will result in the team either proposing a common practice that all can and should agree on, or it will result in the team agreeing that either way is fine and toleration of unimportant inconsistencies (if any). The important concern is that the team be empowered to consider and decide themselves, and that any team member can speak up to raise the issue (both within the code review and in initiating a broader team discussion).

Making the code more obvious and making the code reflect the intent of the author will result in big long-term wins for the product and for the team. Something curious happens once a team gets a little experience at this: they inevitably start forming team norms that lead to more efficient and productive coding. Often it also leads to agreed coding standards that take further time-consuming discussions (or arguments, or fights) out-of-play. These benefits result in snowballing productivity and team happiness as well, so I'll explore that in my next post. I've only scratched the surface on some other benefits of code review, which will also be coming up. 

Do you have a piece of code you'd like to make more readable?  Post it here (maybe a link to a GitHub PR or commit?) and I'll give it a crack.

Code Reviews Are Overrated (but not how you think!)

When I first began leading a "two-pizza" software team in 2007, our code reviews focused on code robustness and correctness. Our architect would choose code to print out--usually around 4-6 pages' worth each week. We would gather in a conference room and spend an hour scrutinizing, and ultimately arguing, about what in that code should change. A few months into the job, he turned over that duty to me and I knew both the printed approach and the goals we were pursuing had to shift! Since that time, tools have greatly improved, code review practices have become easier and more widely known, and the way we structure code is less novel and more relatable. On top of these improvements, developers expect to review code more commonly and with a much healthier attitude; the benefits are more intuitive to all. However, after doing code reviews on most days for the last 17 years, I have come to see that some of the supposed benefits of code review are overrated. Let's acknowledge the limitations of code review toward those goals and consider other goals for which code reviews might be the primary means to gain.

Most importantly, code reviews are not great at finding defects (bugs) in the code. I hesitate to even mention this first: indeed, my teams have used code reviews to find and fix thousands of defects in our code over the years--perhaps making code reviews worthwhile for this benefit alone. Furthermore, bugs found in code review can be fixed much more easily and cheaply than those found in any round of testing, or in the worst case, found by actual users in production. Nevertheless, if we relied on code review as the main way to prevent defects, our quality would dramatically drop. Most defects are simply not obvious enough in code reviews. We need testing in all its forms. We need mutual understanding of requirements, use cases, and/or user stories. We need collaboration and an ongoing conversation about what our software should do and what our users' experience should be. We need an overall team culture growing in software quality and technical excellence. There is not one way to prevent defects; we need all the ways. Certainly code reviews are one tool for higher quality software, but the answer to "why did this bug make it into production?" never comes back as "Eric reviewed the code poorly."

Now let me restate that overrated does not mean nonexistent; let me repeat that code reviews do often find bugs; they just aren't great at doing so. Much as a .300 hitter (that is, a 30% successful hitter) in baseball is an all-star, the value in preventing defects via code review is large despite its failure to detect them more often than not. Keep looking for bugs in your code review effort--just don't expect to find most of them that way.

A concept related to defect prevention is correctness. Does the code do what the author intended it to do? Code reviews are overrated in revealing incorrect code. After all, unless the review somehow expresses what the author intended separately from the code that implements it, the reviewer cannot tell where the intent of the code differs from the code. Reviewers often proceed assuming the best intentions of the author and also assume that the code written accurately reflects those intentions. If a reviewer encounters code they didn't expect, he might think "well, she thought this through and I haven't. She must be right." The wise reviewer will ask questions instead of assuming the best and use the review as a jumping-off point for discussion and collaboration, but no one will be able to question enough or in the right places all the time. Fortunately, there is a proven practice that is explicitly and mainly about checking the code written against the author's intention: unit tests are exactly what we need, especially when they are written side-by-side with the implementation code, either immediately after ("code-then-test") or before (e.g. via TDD). If you aren't using unit testing and instead relying on code reviews to ensure correctness, a lot of misunderstandings and mistakes will slip through to QA or beyond.

The flip side of defect prevention is the completeness and robustness of the code. Does the code do all that it should? Again, code review only examines what the code does. Code review isn't so good at flushing out what the code isn't doing but should be. A really good reviewer will have a strong understanding of the intent and end goals of the code under review and thus can and should question the author on what might be missing. However, sometimes the author is already the person who knows best about what code should be doing, and other reviewers might assume too much given the code they see. Again, use the review as a jumping-off point for discussion and collaboration. But even this healthy attitude will only get you so far. Code reviews are no substitute for a team-wide mutual understanding of the both the user's needs and the big-picture direction of the software effort. It takes many other practices to build the product right; these include product planning time, technical design meetings, pairing, prototyping, and frequent review/discussion with the product owner and users.

Code reviews aren't good at revealing UI/UX shortcomings because code doesn't show the interface or how users interact with it. Testing, especially user-acceptance testing, of any user interface cannot be replaced by code review. A team must engage in UI/UX reviews, both among the team and with their users or user representatives. However, I do highly recommend expanding the idea of "code" reviews to more than code: include mockup/prototype reviews or even reviews of nearly-finished screenshots. Reviewing a screenshot, a UI mockup image, or a photo of a whiteboard is far more valuable than merely reviewing the code that might generate such an interface.

Technical non-coders can't rely on code review to stay in the know. Architects and tech leads who require that all code changes be reviewed by them do their teams a disservice. They are hindering all three of Pink's factors of job happiness: they are blocking their team member's autonomy, preventing them from attaining mastery, and de-emphasizing the purpose of building the software product. At best, they make themselves a necessary impediment to getting the job done. At worst they become the lowest level of rubber-stamping bureaucrats. Instead, architects and tech leads should be sure they are occasionally authoring code just as they did as senior developers, and they should sacrifice time doing code review if necessary to achieve it. By all means architects and tech leads should participate in code reviews (and their recommendations should be valued by all), but the team should be primarily responsible for reviewing each other as peers. The team should be empowered to approve and complete code reviews quickly and to do whatever necessary to prevent code reviews from becoming any kind of delay.

Finally, code review is a poor substitute for cross-training, mutual team learning, and team-wide ownership of the requirements, code, and software deliverables. Code reviews are just too narrow to be our main teaching tool. Because teams that do code reviews well will tend to shorter reviews in higher quantity, any given code review will not expose enough of the software to generate the kind of conversations needed to genuinely spread knowledge around. We can hope that code reviews will help spur on broader conversations about our software, but we cannot depend on them as the main tool for doing so. Furthermore, the deep knowledge needed to grow as a team can only come from implementing or improving that software. Reviewing does not make the reviewer as knowledgeable as its author. Planning, team organization, daily scrums, pairing, and swarming can all help with this. These practices and more will automatically happen the more your team takes on a mindset of collective code ownership.

Thus are you discouraged about the benefits of code review? Don't be. The supposed benefits above can still be had, just not to the extent that you might have hoped for in code reviews We can gain some defect prevention, correctness, robustness, usability, and learning from code reviews; we simply cannot rely mainly on code reviews for these goals. Supplement code reviews with other practices tailored to these goals.

But I haven't yet written of the underrated first-class benefits of code review. Turns out there are many gains for which we can rely on code review--and especially on team peer code review. Compared with my team in 2007, today we review lots more code more efficiently and with different goals. I'll explain a few in my next posts!  Till then, let me know how I can help with your team's code reviews or perhaps review your code myself as freelance side work.

Full-stack? How about "Full Scope"

Hi, I’m Patrick Szalapski.

I make software and help others to make software.

I lead teams to make the most optimally-scoped business web apps and tools, often with .NET, growing such teams to deliver faster, better, and less expensively. I have a proven track record at multiple companies saving businesses millions of dollars and thousands of hours. I can lead or support the effort to figure out what you need, partnering with stakeholders to attack problems and quickly move to building solutions. My teams quickly grow to be highly productive, technically excellent, self-organizing, and happy.

Best of all, I pivot among these roles to whatever a team most needs for sustainable success. 

Scrum Guide updated to include valuing commitment, focus, openness, respect, courage

Recently, Ken Schwaber and Jeff Sutherland updated the official Scrum Guide to include the Scrum values, which are:

• Commitment
• Focus
• Openness
• Respect
• Courage

Sutherland emphasizes that teams adopting Scrum must have a commitment to change and to take your company into the future. Without embracing this change, "you can't get there" and Scrum will be of little value to you.

Schwaber highlighted that focus to work on nothing but the sprint goal will yield the best results; working on other items destroys your productivity. Sutherland noted that doing twice the work in half the time is only possible if the teams focus on the sprint goal alone.

Stakeholders and the team alike need to have a spirit of openness. Sutherland stressed that all happenings must be visible; only when everyone knows what is happening can you best decide how to adjust and how long work will take. We should go so far as not to work on anything that isn't visible.

These are only possible with respect for each other; blame and cover-ups result in poor outcomes. If we respect everyone where they are, they can be empowered to grow and achieve. Sutherland says this applies even to high-pressure cutthroat venture capital-driven startups. Schwaber stresses that respect creates a positive hubbub, a buzz that is charged up with work getting done.

Courage, both to do the right thing and to attempt hard things, is necessary because change is risky. Leaders have a big role to encourage taking wise risks and accepting the downside that sometimes results. Schwaber emphasizes that Scrum provides a safety net--you will never waste more than a sprint's worth of work.

Scrum has the power to make your work life more fun, more balanced, and happier.

View the Scrum Guide: http://www.scrumguides.org/scrum-guide.html - The newest changes are near the beginning, under "Scrum Values".

The ongoing struggle for high-quality unit tests

My team at work has been struggling lately with how ambitious we should be with unit tests. Everyone agrees that we need automated unit tests, and we need to separate them from automated integration tests and run them often. However, I must ask:

How robust should our unit tests be?

There are definitely a few different ways to think about unit testing.  At one extreme is full-on test-driven design (TDD), where every unit of one's software is designed via the unit tests; the unit is responsible for passing all the tests and nothing more. I've never done an actual project using TDD, and I'd like to try--but most teams don't go this far.  The other extreme is to avoid unit tests entirely--everything is tested using integration tests or perhaps manual testing only.  This is not likely good enough for a project of any complexity.

Seems to me that nearly all development projects fall somewhere in between, and my current project is no exception. So we struggle with how robust to make the tests--what functionality needs tests? Is there non-trivial functionality where one might decide to forgo testing in order to work on higher priority things? 

First serious attempt: a "do it right" policy for unit tests

We knew we weren't going to do full TDD. After we realized we needed a better testing policy, we made some rules and guidelines that go something like this:

• Every non-trivial public member of a class or module must have robust unit tests written for it.
• Test every possibly different edge case you can think of.
• Refactor classes so that all dependencies are fully injectible, so that mocked instances can be injected for true unit testing.
• Use a mocking framework like Moq or Microsoft Moles and Stubs to create more fully-featured mock objects with less refactoring.
• Submit every test for code review by at least two other developers (we use CodeCollaborator).
• Reviewers should submit test cases they think are lacking
• Author should follow-up with new tests
• Consider using Pex to help you write tests with better coverage.

Writing high-quality unit tests is difficult, tedious, and unenjoyable

To write unit tests well is to spend a lot of time thinking of what all the possible kinds of inputs are--thinking that developers are more interested in investing in their actual implementation. The tools involved (we used MSTest, Moq, Pex, and Microsoft Moles) all have a learning curve and are not what most developers want to learn.

But it's worth it, we all say, so we soldier on and do our best. Say I've implemented a new feature, and I begin writing unit tests for my new classes.  I stub out each test for a class. By the time I finish a few properties, I have stubbed out and provided focused, detailed thought on each of them. My brain is tired because this thought process is tedious and not the kind of problem-solving thought that makes me engaged and interested. If software design and implementation is akin to doing a jigsaw puzzle, then writing unit tests for that software is akin to doing one's taxes.

I now have much of the work done for these tests I've written, and my software will be better as a result.  Maybe I've even exposed some defects or missing features just by thinking about these tests. Now I am fatigued, but it's worth it, right? But hold on; I'm only done with about a third of the testable members of this class. After I write tests for the remaining two-thirds members, I have to continue on and write tests for several other classes that I've added.

But it's worth it, right? Hold on, I haven't considered the classes that I've added to.  I have to open their existing tests and understand all of the tests that possibly relate to my new feature.  I now have to write a few new unit tests for what I've added to that class. So I repeat the process all over again.  Hopefully, this doesn't take as long, since I only have a few changes.

But it's worth it, right? Now my new feature is unit-tested, and for all time into the future, we will have good assurance that each class works well.  Not so fast--I haven't yet modified the tests for changes to existing behavior to support the new feature. Now I have to examine the existing tests so I can change their preconditions, actions, and/or assertions.  I also have to consider if the test is revealing that my behavior change breaks any existing features.  Maybe my change to existing behavior is naive and we need to reconsider it. 

But it's worth it, right?  Suppose now I am done writing unit tests--what a relief!  But now I need to write integration tests, which feels like starting over for much of the same thing.  And I need to consider if any new backwards-compatibility tests are needed for units that work with older software that can't be upgraded at the same time.


I put my tests through a code review, and the reviewers recommended a few additional cases.  Fortunately, it is usually pretty easy to implement tests for these cases after someone else points them out.

The outcome of this is great, but still has a big flaw: to properly execute the above, it takes a lot of time and determination to get it right. We had tasks that were previously estimated at 4 hours (including perhaps an hour to write tests) and being done in a day now taking perhaps 12 mythical man-hours to be done over more than a week.  Do we have better assurance that our software works well and according to our intent? Without question. But how much? Is it worth taking perhaps double or triple the time to develop something?  We've been developing the "old way" for several months, and our impression was that the business was tentatively happy with our results, including the need for support and patching that arose from a few defects.

After a few months of this style, we begin to degrade. It feels like it is taking too much time to write tests in this way; the business seems more and more unhappy with our decreased velocity (and increased estimates). Team morale is down and excitement for the future wanes.

The perfect is the enemy of the good: tweaking the approach

We are keeping the guidelines, but we will now try be wiser in writing tests--we pick and choose which tests to write, which tests to stub out but leave for writing later, and which units to avoid thinking of tests for at all.  We try to always be "better" (writing more, higher quality, and better covering tests) than we were before, yet we try not to go all-out to do it "right" in every possible way.

So my next emphasis is for all the team members to write robust tests when it is beneficial, and to avoid doing work writing robust tests where they won't be beneficial. I plan to "err on the side of writing more tests", since it is sometimes difficult to know which tests are actually unnecessary--but it seems to me that one lesson from this is that we simply don't have the resources to write "complete" or "really good" unit tests as a hard-and-fast rule.

Relaxing our unit-test vigilance might create some technical debt

I fully acknowledge that there is a potential for issues here. My biggest concern with this approach is that it demands individual judgment by each team member on "how good" to make one's testing.  Code review should mitigate this somewhat, but with an amorphous guideline to avoid writing unit tests that aren't beneficial, no one will know exactly how good "good enough" is.  There is a big risk that as a team we will neglect to write beneficial unit tests for a class because no one who read that code thought writing unit tests were necessary or thought they should speak up.

Another good point is that if we don't spend the time to write unit tests now, or we will spend time troubleshooting later.  This is obviously true to an extent; the sooner you find a bug, the fewer resources it takes to fix or otherwise overcome it.  However, I cannot accept that 100% of the unit tests that one could possibly write will result in an error that we have to find later.  The hope is that we can avoid taking time to write the trivial and "almost trivial" unit tests.  The business has already demonstrated that we are somewhat error-tolerant, as long as we can fix errors reasonably quickly; can we be okay with that as well?

I admit that I am long way from figuring out a "sweet spot" approach to this difficult issue.  How does one discern trivial functionality that needs no testing from non-trivial? How can we ensure that needed unit tests exist? Is there a better, more objective goal that we can aim for in our tests?

If we aim to make our unit tests not "really good" but instead "good enough" given the business demands and resource constraints, will they ever be really good enough?  I'd love to hear all your thoughts and recommended articles; please leave your comments.