Code Reviews Change Over Time

We've been doing code reviews for about 4 years now.

Getting Started with Code Reviews

From the start, developers would help each other out, look at code when someone asked, or sometimes a lead or a senior developer would step in and review code if we were seeing problems in testing or if someone had just joined the team and we anticipated that they needed some extra oversight. We also hired an expert to come in and do a security code review.

After we launched the system, we decided to be proactive and started risk-based reviews: we asked anyone writing code in high-risk areas (like framework and security plumbing, APIs, core business logic, or areas where we had seen problems before) to get a code review.
We could have stopped there, and got a lot of value out of code reviews. But we decided to keep going, and made code reviews a standard practice.

This didn't happen overnight. It wasn't difficult to convince the team that code reviews could help – they’d already seen good results from the risk-based reviews that we had done. But it was difficult to change the way that people worked, and to make sure that they had enough time to do reviews properly: time to schedule and carry out the reviews, and time to understand and act on the feedback. And it also took a while to come up with an effective code review process.

At first we asked developers to pick a buddy and arrange a review. The results were mixed. Sometimes a developer would shop around for someone nice or someone really busy who they figured would let them off lightly; or two developers would trade-off (I’ll show you mine if you’ll show me yours) so they could get the reviews over with as quickly as possible. Because people didn't know how much time they needed to get reviews completed, reviews were often left until too late, and done after the code was already tested and released.

And because most people didn't have a lot of experience doing reviews, they weren't sure what they should look for, and how to give meaningful feedback. Developers were frustrated (he told me to do it this way, but on another change she told me that I should do it that way) and sometimes upset by negative criticism.

Eventually we decided that the leads would do most of the reviews. Although this added a lot more to the leads’ workload, and meant that they couldn't do as much coding themselves, it helped in a few ways. The lead developers usually had a much better understanding of the requirements and what the code was supposed to do than another developer who happened to be available, which mean that they had a better chance of finding real mistakes. And because the same person was doing most of their reviews, developers received consistent feedback.

How we do Code Reviews

How we do reviews has stayed pretty much the same over the years.

Non-trivial code changes are reviewed (either before or after the check-in) regardless of who wrote the code or what the code does.
We don’t hold formal review meetings with a projector or print outs, and we haven’t found it necessary to use a review tool like Code Collaborator or Crucible or the internal tools that Google or Facebook use to manage and track reviews, although looking back adopting a tool like this early might have helped the team start off better.

Sometimes reviews are done face-to-face, but most of the time they’re done offline: the reviewer and developer exchanging information and maybe patch files by email, because we've found this to be more convenient and easier for everyone to schedule (although people will meet to go through the review in detail if they decide it is necessary).

What has changed over time is what the reviewers look for, and what they find.

Reviews for Correctness

It seems like programmers spend more time arguing over what to look for in code reviews than they actually spend doing code reviews.

We started doing code reviews to improve code quality and to find problems that we weren't catching in testing. Not as a way of teaching inexperienced developers how to write better code, or a way to spread knowledge about how the code works across the team. These might be happy side effects, but reviews should be about making sure that the code works right.

Instead of using long check lists, reviewers started off by asking a small set of questions when they looked at code:

1. 
   Does the code do what it is supposed to, and does it look like the logic is correct?
   
   
   
   
   Are there any obvious coding mistakes or code that looks fishy? Is there anything obviously missing or incomplete? What has to be fixed to make sure that the code will work correctly?
   
   
   
   Reviewing for correctness is hard work – a reviewer has to spend some time understanding the requirements, and more time thinking about the code and trying to get into the head of the person who wrote it.
   
   
   
   But even without a lot of time or familiarity, a good reviewer can still see glaring logical mistakes and oversights, inconsistencies (you changed a, b, and d, but you didn't change c. Was this on purpose?), common coding mixups (looking out for < instead of <= or sometimes > in comparisons, off-by-one errors, using the wrong variables in calculations or comparisons – buyer instead of seller, inviter instead of invitee), debugging code left on by accident, redundant code and checks that don’t seem to be needed and other suspect code that doesn't look right or is confusing.
   
   
   
   Reviewers can also look out for basic security issues (access control rules/roles being applied properly, correct handling of sensitive data), backwards compatibility, and proper use of APIs and language features (especially for people new to the team).
   
   
   
   And while we don’t ask reviewers to spend a lot of time reviewing automated tests, they may also do spot checks to look for holes in the test suite – especially if they find an obvious coding mistake (which means that some tests are either missing or wrong).
   
   
   
   


2. 
   Does this code look the same and work the same as the rest of the code base?
   
   
   
   Does it follow the style and conventions that the team has agreed to (indentation, headers, capitalization and naming conventions, import sequence…)? Does it use standard templates and auto-formatting rules? We’re not talking about nitpicky perfection here, but we don’t want every piece of code to look different either.
   
   
   
   Architectural conventions. Does it use the framework and common libraries properly? Does it properly follow the big patterns (MVC etc) that we want people to understand and use?
   
   
   
   
   We wanted developers to work out issues around consistency and style early on while the team was coming together and everyone was still learning and exploring. This turned out to take longer and cause more trouble than we expected. Not only is it tedious to check for, but style can also be a near religious thing for some people. A developer won’t argue over whether something is a bug or not, at least not after they've understood what’s wrong and why. And most developers appreciate learning how to use the framework or language more effectively, as long as this is done in a polite and respectful way. But some people get seriously wound up over aesthetics and style, what kind of code looks cleaner or is more elegant.
   

Reviewers could offer other feedback of course, advice and suggestions on how to simplify the solution and improve the code, but we left it up to the developer when or if to act on this advice as long as the code is correct and follows conventions.

Reviewing for Understandability

Over time, what you look for and what you get out of code reviews should change. Because the code has changed. And the people doing the work – the developers and the reviewers – have changed too.

It got easier for reviewers to find bugs and bad code as developers learned more about how to use the code checkers in their IDEs and after we found some good static analysis tools to automatically check for common coding bugs and bad coding practices. This mean that reviewers could spend their time looking for more important design mistakes, or timing errors or sequence accounting errors or concurrency bugs like race conditions and potential deadlocks, and other problems that the tools can’t find.

As the team gained more experience supporting the system in production, troubleshooting and fixing problems and working with Ops, reviewers started to look more closely at defensive programming and run-time safety: error and exception handling (especially exception conditions that are difficult to test for), data validation and limits checking, timeouts and retries, properly enforcing API contracts, and logging and alerting. Making sure that the system “wouldn't boink”.

Because we worked out most of the consistency issues early, it stopped being necessary to review for consistency after a while. There was lots of code, so it was natural and easy for developers to build on what was already there, and to keep following the existing style and patterns.

But this also meant that there was a lot more code that had to be read and understood. So code reviewers became more concerned with anything that made the code harder to read and understand. Although fussing over details like method and variable names and whether comments are needed or make sense might not seem that important, all of this feeds back into correctness – reviewers can’t tell if the code is correct if they aren't sure that they really understand it.

Reviewers also started looking out more for unnecessary code duplication because clones create more opportunities for mistakes when you have to make changes, and code that that hasn't been updated to use new libraries or APIs, or code that isn't needed any more (especially a problem if you use feature switches and don’t get around to removing them),
or refactoring that didn't get finished or that got out of control.

Understanding, not Criticizing

Reviews are about understanding the code, and making sure that it works right, not criticizing it.

It’s important to stay out of arguments about “what I think good code is vs. what you think good code is”. A review should never run along the lines of “I know what good OO code is supposed to look like, and apparently you don’t, so let me show you.”

A review should only be about “I have to be able to understand this code well enough to make sure that it works, and in case I have to fix something in this code myself, even though I wouldn't have written it this way and wish that you hadn't (but I’ll keep this part to myself).”

When reviewers step across this line, when people’s pride and sense of professional self-worth are involved, things can get ugly fast.

Besides avoiding arguments, it’s a waste of time giving feedback and suggestions that nobody is going to act on because they’re too busy to get to it or they don’t agree or it isn't necessary. If you want to get good value out of reviews, keep them focused on what’s important.

The main questions that reviewers try to answer now when they look at code are:

1. 
   Does this code do what it is supposed to do? And will it keep working even if something goes wrong?
   
   
   
   Reviewers keep looking for the same kinds of obvious logic and functional problems as they did before. But they also ask: What happens if it runs into an error? Will Ops be able to tell what the problem was? What happens if some other part of the system that it relies on fails? Will it retry and recover, will it fail gracefully, or will it go boom? What’s the chance that this code could get garbage passed to it, and what would happen if it did? And can it pass garbage to some other part of the system?
   
   
   
   


2. 
   Can I be sure that this code does what it is supposed to do?
   Can I understand the code? Can I follow the changes that were made?
   
   
   
   Is the code maintainable?
   Could I fix it in production if something went wrong? Is there any code too that is cute or too clever for its own good? Are there any obvious, safe and simple things that can be done to make the code simpler and easier to understand and easier to change?
   

Some things get better with time, other things don’t

By looking at changes to code bases over time, Michael Feathers has found that most code is rarely or never changed once it is written; most changes are made to over and over to the same small percentage of the code base.

Because reviewers keep seeing the same code being changed, it changes how they work:

1. 
   Reviewers who have seen the code before don’t have to spend as much time figuring out what’s going on, they understand the context and have a better chance of understanding the changes and whether the changes are done correctly.
   
   
   
   Looking at the same code again, reviewers may see legacy problems that they didn't notice before. And they should be able to offer more valuable, less superficial feedback on how the code can be simplified and improved.
   
   
   
   There’s a trade-off being made here too of course. Reviewers can also get stale looking at the same code, and stop looking closely enough. And the reviewer’s biases and blind spots will get amplified over time.
   
   
   
   


2. 
   As reviewers work with the same developers over and over, they will get to know each other better. Reviewers will learn what to look out for, what the developer’s strengths and weaknesses are, the developer’s tendencies and biases and blind spots, the kinds of mistakes and oversights that they tend to make.
   
   
   
   As developers get to know the reviewers better, feedback will get shorter and simpler, and there will be fewer misunderstandings.
   
   
   
   And because developers will know more about what to expect from a review, they will start to check their own code more carefully, and try to find the problems before the reviewer does. This is what Jason Cohen at SmartBear calls “the Ego Effect”: developers write better code because they know that somebody is going to be looking at it closely, especially if it’s someone they respect. Some people feel that this is the most important reason to do code reviews – to make developers work more carefully, so they don’t look foolish in front of somebody else; and to think about writing code that other people will read.
   
   

Avoiding Diminishing Returns

Like any practice in software development, you will eventually see diminishing returns from code reviews especially if you keep doing the same things, looking for the same problems in the same way. You may even reach a point where code reviews aren't worth the cost or time.

This hasn't happened to us. We continue to get a lot of value out of reviews, maybe even more now.

Even as our tools have improved and as we've strengthened our testing capability we keep doing reviews
because bug checking tools, reviews and testing find different problems and different kinds of problems.

We've also found that reviews make testing more effective and efficient, because there’s less back-and-forth between developers and testers over mistakes that are caught and corrected earlier.

And now that reviews are an accepted part of how people work, we don’t have to spend time selling and re-selling the value of code reviews to the team or to the business.
As long as code reviews are being taken seriously, and as long as you care about writing good code, they’re worth doing.

What does Code Ownership do to Code?

In my last post, I talked about Code Ownership models, and why you might want to choose one code ownership model (strong, weak/custodial or collective) over another. Most of the arguments over code ownership focus on managing people, team dynamics, and the effects on delivery. But what about the longer term effects on the shape, structure and quality of code – does the ownership model make a difference? What are the long-term effects of letting everyone working on the same code, or of having 1 or 2 people working on the same pieces of code for a long time?

Collective Code Ownership and Code Quality

Over time, changes tend to concentrate in certain areas of code: in core logic and in and behind interfaces (listen to Michael Feathers’ fascinating talk Discovering Startling Things from your Version Control System). This means that the longer a system has been running, the more chances there are for people to touch the same code.
Some interesting research work backs up what should be obvious: that the people who understand the code the best are the people who work on it the most, and the people who know the code the best make less mistakes when changing it.

In Don’t Touch my Code!, researchers at Microsoft (BTW, the lead author Christian Bird is not a relative of mine, at least not a relative who I know) found that as more people touch the same piece of code, it leads to more opportunities for misunderstandings and more mistakes. Not surprisingly, people who hadn't worked on a piece of code before made more mistakes, and as the number of developers working on the same module increased, so did the chance of introducing bugs.

Another study, Ownership and Experience in Fix-Inducing Code tries to answer which is more important in code quality: “too many cooks spoil the broth”, or “given enough eyeballs, all bugs are shallow”? Does more people working on the same code lead to more bugs, or does having more people working on the code mean that there are more chances to find bugs early? This research team found that a programmer’s specific experience with the code was the most important factor in determining code quality – code that is changed by the programmer who does most of the work on that code is of higher quality than code written by someone who doesn't normally work on the code, even if that someone is a senior developer who has worked on other parts of the code. And they found that the fewer the people working on a piece of code, the fewer the bugs that needed to be fixed.

And a study on contributions to Linux reinforces that as the number of developers working on the same piece of code increase, the chance of bugs and security problems increases significantly: code touched by more than 9 developers is 16x more likely to have security vulnerabilities, and more vulnerabilities are introduced by developers who are making changes across many different pieces of code.

Long-term Effects of Ownership Approach on Code Structure

I've worked at shops where the same programmers have owned the same code for 3 or 4 or 5 or even 10 years or sometimes even longer. Over that time, that programmer’s biases, strengths, weaknesses and idiosyncrasies are all amplified, wearing deep grooves in the code. This can be a good thing, and a bad thing.

The good thing is that with one person making most or all of the changes, internal consistency in any piece of code will be high – you can look at a piece of code written by that developer and once you understand their approach and way of thinking, the patterns and idioms that they prefer, everything should be familiar and easy to follow. Their style and approach might have changed over time as they learned and improved as a developer, but you can generally anticipate how the rest of the code will work, and you’ll recognize what they are good at and what their blind spots are, what kind of mistakes they are prone to: as I mentioned in the earlier post, this makes code easier to review and easier to test and so easier to find and fix bugs.

If a developer tends to write good, clean, tight code, and if they are diligent about refactoring and keeping the code clean and tight, then most of the code will be good, clean, tight and easy to follow. Of course it follows that if they tend to write sloppy, hard-to-understand, poorly structured code, then most of it will be sloppy, hard-to-understand and poorly-structured. Then again, even this can be a good thing – at least bad code is isolated, and you know what you have to rewrite, instead of someone spreading a little bid of badness everywhere.

When ownership changes – when the primary contributor leaves, and a new owner takes over, the structure and style of the code will change as well. Maybe not right away, because a new owner usually takes some time to get used to the code before they put their stamp on it, but at some point they’ll start adapting it – even unconsciously – to their own preferences and biases and ways of thinking, refactoring or rewriting it to suit them.

If a lot of developers have worked on the same piece of code, they will introduce different ideas, techniques and approaches over time as they each do their part, as they refactor and rewrite things according to their own ideas of what is easy to understand and what isn't, what’s right and wrong. They will each make different kinds of mistakes. Even with clear and consistent shared team conventions and standards, differences and inconsistencies can build up over time, as people leave and new people join the team, creating dissonance and making it harder to follow a thought through the code, harder to test and review, and harder to hold on to the design.

Ownership Models and Refactoring

But as Michael Feathers has found through mining version control history, there is also a positive Ownership Effect on code as more people work on the same code.

Over time, methods and classes tend to get bigger because it’s easier to add code to an existing method than to write a new method, and easier to add another method to an existing class than create a new class. By correlating the number of developers who have touched a piece of code with method size, Feathers research shows that as the number of developers working on a piece of code increases, the average method size tends to get smaller. In other words, having multiple people working on a code base encourages refactoring and simpler code, because people who aren't familiar with the code have to simplify it first in order to understand it.

Feathers has also found that code behind APIs tends to be especially messy – because some interfaces are too hard to change, programmers are forced to come up with their own workarounds behind the scenes. Martin Fowler explains how this problem is made worse by strong code ownership, which inhibits refactoring and makes the code more internally rigid:

In strong code ownership, there's my code and your code. I can't change your code. If I want to change the name of one of my methods, and it's called by your code, I've got to get you to change the call into me before I can change my name. Or I've got to go through the whole deprecation business. Essentially any of my interfaces that you use become published in that situation, because I can't touch your code for any reason at all.



There's an intermediate ground that I call weak code ownership. With weak code ownership, there's my code and your code, but it is accepted that I could go in and change your code. There's a sense that you're still responsible for the overall quality of your code. If I were just going to change a method name in my code, I'd just do it. But on the other hand, if I were going to move some responsibilities between classes, I should at least let you know what I'm going to do before I do it, because it's your code. That's different than the collective code ownership model.



Weak code ownership and refactoring are OK. Collective code ownership and refactoring are OK. But strong code ownership and refactoring are a right pain in the butt, because a lot of the refactorings you want to make you can't make. You can't make the refactorings, because you can't go into the calling code and make the necessary updates there. That's why strong code ownership doesn't go well with refactoring, but weak code ownership works fine with refactoring.


(Design Principles and Code Ownership)

Ownership, Technical Debt or Deepening Insight

An individual owner has a higher tolerance for complexity, because after all it’s their code and they know how it works and it’s not really that hard to understand (not for them at least) so they don’t need to constantly simplify it just to make a change or fix something. It's also easy for them to take short cuts, and even short cuts on short cuts. This can build up over time until you end up with a serious technical debt problem – one person is always working on that code, not because the problem is highly specialized, but because the code has reached a point where nobody else but Scotty can understand it and make it work.

There’s a flip side to spending more time on code too. The more time that you spend on the same problem, the deeper you can see into it. As you return to the same code again and again you can recognize patterns, and areas that you can improve, and compromises that you aren't willing to accept any more. As you learn more about the language and the frameworks, you can go back and put in simpler and safer ways of doing things. You can see what the design really should be, where the code needs to go, and take it there.

There's also opportunity cost of not sticking to certain areas. Focusing on a problem allows you to create better solutions. Specifically, it allows you to create a vision of what needs to be done, work towards that vision and constantly revise where necessary... If you're jumping from problem to problem, you're more likely to create an inferior solution. You'll solve problems, but you'll be creating higher maintenance costs for the project in the long term.

Jay Fields Taking a Second Look at Collective Code Ownership

So far I've found that the only way for a team to take on really big problems is by breaking the problems up and letting different people own different parts of the solution. This means taking on problems and costs in the short term and the long term, trading off quality and productivity against flexibility and consistency – not only flexibility and consistency in how the team works, but in the code itself.

What I've also learned is that whether you have a team of people who each own a piece of the system, or a more open custodian environment, or even if everyone is working everywhere all of the time, you can’t let people do this work completely on their own. It’s critical to have people working together, whether you are pairing in XP or doing regular egoless code reviews. To help people work on code that they’ve never seen before – or to help long-time owners recognize their blind spots. To mentor and to share new ideas and techniques. To keep people from falling into bad habits. To keep control over complexity. To reinforce consistency – across the code base or inside a piece of code.

Code Ownership – Who Should Own the Code?

A key decision in building and managing any development team is agreeing on how ownership of the code will be divided up: who is going to work on what code; how much work can be, and should be, shared across the team; and who will be responsible for code quality. The approach that you take has immediate impact on the team’s performance and success, and a long-term impact on the shape and quality of the code.

Martin Fowler describes three different models for code ownership on a team:

1. Strong code ownership – every module is owned exclusively by someone, developers can only change the code that they own, and if they need to change somebody else’s code, they need to talk to that owner and get the owner’s agreement first – except maybe in emergencies.
   
   
   
   
   
2. Weak code ownership – where modules are still assigned to owners, but developers are allowed to change code owned by other people. Owners are expected to keep an eye on any changes that other people make, and developers are expected to ask for permission first before making changes to somebody else’s code.
   
   
   
   
   
   This can be thought of as a shared custody model, where an individual is forced to share ownership of their code with others; or Code Stewardship, where the team owns all of the code, but one person is held responsible for the quality of specific code, and for helping other people make changes to it, reviewing and approving all major changes, or pairing up with other developers as necessary. Brad Appleton says the job of a code steward is not to make all of the changes to a piece of code, but to
   “safeguard the integrity + consistency of that code (both conceptually and structurally) and to widely disseminate knowledge and expertise about it to others”.
   
   
   
   
   
3. Collective Code Ownership – the code base is owned or shared by the entire team, and everyone is free to make whatever changes they need – or want – to make, including refactoring or rewriting code that somebody else originally wrote. This is a model that came out of Extreme Programming, where the Whole Team is responsible together for the quality and integrity of the code and for understanding and keeping the design.
   

Arguments against Strong/Individual Code Ownership

Fowler and other XP advocates such as Kent Beck don’t like strong individual code ownership, because it creates artificial barriers and dependencies inside the team. Work will stall and pause if you need to wait for somebody to make or even approve a change, and one owner can often become the critical path for the entire team. This could encourage developers to come up with their own workarounds and compromises. For example, instead of changing an API properly (which would involve a change to somebody else’s code), they might shoe horn in a change, like stuffing something into an existing field. Or they might take a copy of somebody’s code and add whatever they need to it, making maintenance harder in the future.

Other arguments against strong ownership are that it can lead to defensiveness and protectionism on the part of some developers (“hey, don’t touch my code!”), where they take any criticism of the code as a personal attack, creating tension on the team and discouraging reviewers from offering feedback and discouraging refactoring efforts; and local over-optimization, if developers are given too much time to spend to polish and perfect their precious code without thinking of the bigger picture.

And of course there is the “hit by a truck factor” to consider – the impact that a person leaving the team will have on productivity if they’re the only one who works on a piece of code.

Ward Cunningham. one of the original XPers, also believes that there is more pride of ownership when code is shared, because everyone’s work is always on display to everyone else on the team.

Arguments against Collective Code Ownership

But there are also arguments against Collective Code Ownership. A post by Mike Spille lists some problems that he has seen when teams try to “over-share” code:

• 
  Inconsistency. No overriding architecture is discernible, just individual solutions to individual problems. Lots of duplication of effort results, often leading to inconsistent behavior
  
• 
  Bugs. People "refactoring" code they don't really understand break something subtle in the original code.
  
• 
  Constant rounds of "The Blame Game". People have a knee jerk reaction to bugs, saying "It worked when I wrote it, but since Joe refactored it....well, that's his problem now.".
  
• 
  Slow delivery. Nobody has any expertise in any given domain, so people are spending more time trying to understand other people's code, less time writing new code.
  

Matthias Friedrich, in Thoughts on Collective Code Ownership believes that Collective Code Ownership can only work if you have the right conditions in place:

• 
  Team members are all on a similar skill level
  
• 
  Programmers work carefully and trust each other
  
• 
  The code base is in a good state
  
• 
  Unit tests are in place to detect problematic changes (although unit tests only go so far)
  

Remember that Collective Code Ownership came out of Extreme Programming. Successful team ownership depends on everyone sharing an understanding of the domain and the design, and maintaining a high-level of technical discipline: not only writing really good automated tests as a safety net, but everyone following consistent code conventions and standards across the code base, and working in pairs because hopefully one of you knows the code, or at least with two heads you can try to help each other understand it and make fewer mistakes.

Another problem with Collective Code Ownership is that ownership is spread so thin. Justin Hewlett talks about the Tragedy of the Commons problem: people will take care of their own yard, but how many people will pick up somebody else’s litter in the park, or on a street - even if they walk in that park or down that street everyday? If the code belongs to everyone, then there is always “someone else” who can take care of it – whoever that “someone else” may be. As a developer, you’re under pressure, and you may never touch this piece of code again, so why not get whatever you need to do as quickly as possible and get on to the next thing on your list, and let "somebody else" worry about refactoring or writing that extra unit test or...?

Code Ownership in the Real World

I've always worked on or with teams that follow individual (strong or weak) code ownership, except for an experiment in pure XP and Collective Code Ownership on one team over 10 years ago. One (or maybe two) people own different pieces of the code and do all or most of the heavy lifting work on that code. Because it only makes sense to have the people who understand the code best do most of the work, or the most important work. It’s not just because you want the work “done right” – sometimes you don’t really have a choice over who is going to do the work.

As Ralf Sudelbucher points out, Collective Code ownership assumes that all coding work is interchangeable within a team, which is not always true.

Some work isn't interchangeable because of technology: different parts of a system can be written in different languages, with different architectures. You have to learn the language and the framework before you can start to understand the other problems that need to be solved.

Or it might be because of the problem space. Sure, there is always coding on any project that is “just typing”: journeyman work that is well understood, like scaffolding work or writing another web form or another CRUD screen or fixing up a report or converting a file format, work that has to be done and can be taken on by anyone who has been on the team for a while and who understands where to find stuff and how things are done – or who pairs up with somebody who knows this.

But other software development involves solving hard domain problems and technical problems that require a lot of time to understand properly – where it can take days, weeks, months or sometimes even years to immerse yourself in the problem space well enough to know what to do, where anyone can’t just jump in and start coding, or even be of much help in a pair programming situation.

The worst disasters occur when you turn loose sorcerers' apprentices on code they don't understand. In a typical project, not everyone can know everything - except in some mature domains where there have been few business paradigm shifts in the past decade or two.


Jim Coplien, Code Ownership

I met someone who manages software development for a major computer animation studio. His team has a couple of expert developers who did their PHDs and post grad work in animating hair – that’s all that they do, and even if you are really smart you’ll need years of study and experience just to understand how they do what they do.

Lots of scientific and technical engineering domains are also like this – maybe not so deeply specialized, but they involve non-trivial work that can’t be easily or competently done by generalists, even competent generalists. Programming medical devices or avionics or robotics or weapons control; or any business domain where you are working at the leading edge of problem solving, applying advanced statistical models to big data analysis or financial trading algorithms or risk-management models; or supercomputing and high-scale computing and parallel programming,
or writing an operating system kernel or solving cryptography problems or doing a really good job of User Experience (UX) design. Not everyone understands the problems that need to be solved, not everyone cares about the problems and not everyone can do a good job of solving them.

Ownership and Doing it Right

If you want the work done right, or need it to be done right the first time, it should be done by someone who has worked on the code before, who knows it and who has proven that they can get the job done. Not somebody who has only a superficial familiarity with the code. Research work by Microsoft and others have shown that as more people touch the same piece of code, there is more chance of misunderstandings and mistakes – and that the people who have done the most work on a piece of code are the ones who make the fewest mistakes.

Fowler comes back to this in a later post about “Shifting to Code Ownership” where he shares a story from a colleague who shifted a team from collective code ownership to weak individual code ownership because weaker or less experienced programmers were making mistakes in core parts of the code and impacting quality, velocity and the team’s morale. They changed their ownership model so anyone could work around the code base, but if they needed to change core code, they had to do this with the help of someone who knew that part of the code well.

In deciding on an an ownership approach, you have to make a trade-off between flexibility and quality, team ownership and individual ownership. With individual ownership you can have siloing problems and dependencies on critical people, and you’ll have to watch out for trucks. But you can get more done, faster, better and by fewer people.

Architecture-Breaking Bugs – when a Dreamliner becomes a Nightmare

The history of computer systems is also the history of bugs, including epic, disastrous bugs that have caused millions of $ in damage and destruction and even death, as well as many other less spectacular but expensive system and project failures. Some of these appear to be small and stupid mistakes, like the infamous Ariane 5 rocket crash, caused by a one-line programming error. But a one-line programming error, or any other isolated mistake or failure, cannot cause serious damage to a large system, without fundamental failures in architecture and design, and failures in management.

Boeing's 787 Dreamliner Going Nowhere


The Economist, Feb 26 2013

These kinds of problems are what Barry Boehm calls “Architecture Breakers”: where a system’s design doesn't hold up in the real world, when you run face-first into a fundamental weakness or a hard limit on what is possible with the approach that you took or the technology platform that you selected.

Architecture Breakers happen at the edges – or beyond the edges – of the design, off of the normal, nominal, happy paths. The system works, except for a “one in a million” exceptional error, which nobody takes seriously until a “once in a million” problem starts happening every few days. Or the system crumples under an unexpected surge in demand, demand that isn't going to go away unless you can’t find a way to quickly scale the system to keep up – and if you can’t, you won’t have a demand problem any more because those customers won’t be coming back. Or what looks like a minor operational problem turns out to be the first sign of a fundamental reliability or safety problem in the system.

Dreamliner is Troubled by Questions about Safety


NY Times, Jan 10, 2013

Finding Architecture Breakers

It starts off with a nasty bug or an isolated operational issue or a security incident. As you investigate and start to look deeper you find more cases, gaping holes in the design, hard limits to what the system can do, or failures that can’t be explained and can’t be stopped. The design starts to unravel as each problem opens up to another problem. Fixing it right is going to take time and money, maybe even going back to the drawing board and revisiting foundational architectural decisions and technology choices. What looked like a random failure or an ugly bug just turned into something much uglier, and much much more expensive.

Deepening Crisis for the Boeing 787


NY Times, Jan 17 2013

What makes these problems especially bad is that they are found late, way past design and even past acceptance testing, usually when the system is already in production and you have a lot of real customers using it to get real work done. This is when you can least afford to encounter a serious problem. When something does go wrong, it can be difficult to recognize how serious it is right away. It can take two or three or more failures before you realize – and accept – how bad things really are and before you see enough of a pattern to understand where the problem might be.

Boeing Batteries Said to Fail 10 Times Before Incident


Bloomberg, Jan 30 2013

By then you may be losing customers and losing money and you’re under extreme pressure to come up with a fix, and nobody wants to hear that you have to stop and go back and rewrite a piece of the system, or re-architect it and start again – or that you need more time to think and test and understand what’s wrong and what your options are before you can even tell them how long it might take and how much it could cost to fix things.

Regulators Around the Globe Ground Boeing 787s


NY Times, Jan 18 2013

What can Break your Architecture?

Most Architecture Breakers are fundamental problems in important non-functional aspects of a system:

• 
  Stability and data integrity: some piece of the system won’t stay up under load or fails intermittently after the system has been running for hours or days or weeks, or you lost critical customer data or you can’t recover and restore service fast enough after an operational failure.
  
• 
  Scalability and throughput: the platform (language or container or communications fabric or database – or all of them) are beautiful to work with, but can’t keep up as more customers come in, even if you throw more hardware at it. Ask Twitter about trying to scale-out Ruby or Facebook about scaling PHP or anyone who has ever tried to scale-out Oracle RAC.
  
• 
  Latency – requirements for real-time response-time/deadline satisfaction escalate, or you run into unacceptable jitter and variability (you chose Java as your run-time platform, what happens when GC kicks in?).
  
• 
  Security: you just got hacked and you find out that the one bug that an attacker exploited is only the first of hundreds or thousands of bugs that will need to be found and fixed, because your design or the language and the framework that you picked (or the way that you used it) is as full of security holes as Swiss cheese.
  

These problems can come from misunderstanding what an underlying platform technology or framework can actually do – what the design tolerances for that architecture or technology are. Or from completely missing, overlooking, ignoring or misunderstanding an important aspect of the design.

These aren’t problems that you can code your way out of, at least not easily. Sometimes the problem isn't in your code any ways: it’s in a third party platform technology that can’t keep up or won’t stay up. The language itself, or an important part of the stack like the container, database, or communications fabric, or whatever you are depending on for clustering and failover or to do some other magic. At high scale in the real world, almost any piece of software that somebody else wrote can and will fall short of what you really need, or what the vendor promised.

Boeing, 787 Battery Supplier at Odds over Fixes


Wall Street Journal, Feb 27 2013

You’ll have to spend time working with a vendor (or sometimes with more than one vendor) and help them understand your problem, and get them to agree that it’s really their problem, and that they have to fix it, and if they can’t fix it, or can’t fix it quickly enough, you’ll need to come up with a Plan B quickly, and hope that your new choice won’t run into other problems that may be just as bad or even worse.

How to Avoid Architecture Breakers

Architecture Breakers are caused by decisions that you made early and got wrong – or that you didn't make early enough, or didn't make at all. Boehm talks about Architecture Breakers as part of an argument against Simple Design – that many teams, especially Agile teams, spend too much time focused on the happy path, building new features to make the customer happy, and not enough time on upfront architecture and thinking about what could go wrong. But Architecture Breakers have been around a lot longer than Agile and simple design: in Making Software (Chapter 10 Architecting: How Much and When), Boehm goes back to the 1980s when he first recognized these kinds of problems, when Structured Programming and later Waterfall were the “right way” to do things.

Boehm’s solution is more and better architecture definition and technical risk management through Spiral software development: a lifecycle with architecture upfront to identify risk areas, which are then explored through iterative, risk-driven design, prototyping and development in multiple stages. Spiral development is like today’s iterative, incremental development methods on steroids, using risk-based architectural spikes, but with much longer iterative development and technical prototyping cycles, more formal risk management, more planning, more paperwork, and much higher costs.

Bugs like these can’t all be solved by spending more time on architecture and technical risk management upfront – whether through Spiral development or a beefed up, disciplined Agile development
approach. More time spent upfront won`t help if you make naïve assumptions about scalability, responsiveness or reliability or security; or if you don’t understand these problems well enough to identify the risks. Architecture Breakers won’t be found in design reviews – because you won’t be looking for something that you don’t know could a problem – unless maybe you are running through structured failure modelling exercises like FMEA (Failure mode and effect analysis) or FMECA (Failure mode, effects and criticality analysis), which force you to ask hard questions, but which few people outside of regulated industries have even heard about.

And Architecture Breakers can't all be caught in testing, even extended longevity/soak testing and extensive fuzzing and simulated failures and fault injection and destructive testing and stress testing – even if all the bugs that are found this way are taken seriously (because these kinds of extreme tests are often considered unrealistic).

You have to be prepared to deal with Architecture Breakers. Anticipating problems and partitioning your architecture using something like the Stability Patterns in Michael Nygard’s excellent book Release It! will at least keep serious run-time errors from spreading and taking an entire system out (these strategies will also help with scaling and with containing security attacks). And if and when you do see a “once in a million” error in reviews or testing or production, understand how serious it can be, and act right away – before a Dreamliner turns into a nightmare.

War Games, Pair Testing and Other Fun Ways to Find Bugs

I've already examined how important good testing is to the health of a project, a product and an organization. There’s a lot more to good testing than running an automated test suite in Continuous Integration and forcing someone to walk through functional test scripts and check lists. A good tester will spend time exploring the app, making sure that they really understand it and that the app actually makes sense, finding soft spots and poking them to uncover problems that nobody expects, providing valuable information and feedback to the team.

What’s better than a good tester? Two good testers working together…

Pair Testing – Two Heads are Better than One

Pair Testing is an exploratory testing approach where two testers work through scenarios together, combining their knowledge of the app and their unique skills and experience to duplicate hard-to-find bugs or to do especially deep testing of some part of a system. Like in pair programming, one person drives, defining the goals of the testing session, the time limit and the starting scenarios and providing the hands at the keyboard; and the other person navigates, observes, takes notes, advises, asks questions, double checks, challenges and causes trouble. As a pair they can help each other through misunderstandings and blocks, build on each other’s ideas to come up with new variations and more ways to attack the app, push each other to find more problems, and together they have a better chance of noticing small inconsistencies and errors that the other person might not consider important.

Pair testing can be especially effective if you pair developers and testers together – a good tester knows where to look for problems and how to break software; a good developer can use their understanding of the code and design to suggest alternative scenarios and variations, and together they can help each other recognize inconsistencies and identify unexpected behaviour. This is not just a good way to track down bugs – it’s also a good way for people to learn from each other about the app and about testing in general. In our team, developers and testers regularly pair up to review and test hard problems together, like validating changes to complex business rules or operational testing of distributed failover and recovery scenarios.

Pair testing, especially pairing developers and testers together, is a mature team practice. You need testers and developers who are confident and comfortable working together, who trust and respect each other, who understand the value and purpose of exploratory testing, and who are all willing to put the time in to do a good job.

War Games and Team Testing

If two heads are better than one, then what about four heads, or eight, or ten or …?

You can get more perspectives and create more chances to learn by running War Games: team testing sessions which put a bunch of people together and try to get as close as possible to recreating real-life conditions. In team testing, one person defines the goals, roles, time limit and main scenarios. Multiple people end up driving, each playing different roles or assuming different personas, some people trying crazy shit to see what happens, others being more disciplined, while somebody else shoulder surfs or looks through logs and code as people find problems. More people means more variations and more chances to create unexpected situations, more eyes to look out for inconsistencies and finishing details (“is the system supposed to do this when I do that?”), and more hands to try the same steps at the same time to test for concurrency problems. At worst, you’ll have a bunch of monkeys bashing at keyboards and maybe finding some bugs. But a well-run team test session is a beautiful thing, where people feed on each other’s findings and ideas and improvise in a loosely structured way, like a jazz ensemble.

Testing this way makes a lot of sense for interactive systems like online games, social networks, online stores or online trading: apps that support different kinds of users playing different roles with different configurations and different navigation options that can lead to many different paths through the app and many different experiences.

With so many people doing so many things, it’s important that everyone (or at least someone) has the discipline to keep track of what they are doing, and make notes as they find problems. But even if people are keeping decent notes, sometimes all that you really know is that somebody found a problem, but nobody is sure what exactly they were doing at the time or what the steps are to reproduce the problem. It can be like finding a problem in production, so you need to use similar troubleshooting techniques, rely more on logs and error files to help retrace steps.

Team testing can be done in large groups, sometimes even as part of acceptance testing or field testing with customers. But there are diminishing returns: as more people get involved, it’s harder to keep everyone motivated and focused, and harder to understand and deal with the results. We used to invite the entire team into team testing sessions, to get as many eyes as possible on problems, and to give everyone an opportunity to see the system working as a whole (which is important when you are still building it, and everyone has been focused on their pieces).

But now we've found that a team as small as four to six people who really understand the system is usually enough, better than two people, and much more efficient than ten, or a hundred. You need enough people to create and explore enough options, but a small enough group that everyone can still work closely together and stay engaged.

Team testing is another mature team practice: you need people who trust each other and are comfortable working together, who are reasonably disciplined, who understand exploratory testing and who like finding bugs.

Let's Play a Game

We relied on War Games a lot when we were first building the system, before we had good automated testing coverage in place. It was an inefficient, but effective way to increase code coverage and find good bugs before our customers did.

We still rely on War Games today, but now it’s about looking for real-life bugs: testing at the edges, testing weird combinations and workflow chaining problems, looking closely for usability and finishing issues, forcing errors, finding setup and configuration mistakes, and hunting down timing errors and races and locking problems.

Team testing is one of the most useful ways to find subtle (and not so subtle) bugs and to build confidence in our software development and testing practices. Everyone is surprised, and sometimes disappointed, by the kinds of problems that can be found this way, even after our other testing and reviews have been done. This kind of testing is not just about finding bugs that need to be fixed: it points out areas where we need to improve, and raises alarms if too many – or any scary – problems are found.

This is because War Games only make sense in later stages of development, once you have enough of a working system together to do real system testing, and after you have already done your basic functional testing and regression. It’s expensive to get multiple people together, to set up the system for a group of people to test, to define the roles and scenarios, and then to run the test sessions and review the results – you don’t want to waste everyone’s time finding basic functional bugs or regressions that should have and could have been picked up earlier. So whatever you do find should be a (not-so-nice) surprise.

War Games can also be exhausting – good exploratory testing like this is only effective if everyone is intensely involved, it takes energy and commitment. This isn’t something that we do every week or even every iteration. We do it when somebody (a developer or a tester or a manager) recognizes that we’ve changed something important in workflow or the architecture or business rules; or decides that it’s time, because we’ve made enough small changes and fixes over enough iterations or because we’ve seen some funny bugs in production recently, time to run through key scenarios together as a group and see what we can find.

What makes War Games work is that they are games: an intensity and competition builds naturally when you get smart people working together on a problem, and a sense of play.

“Framing something like software testing in terms of gaming, and borrowing some of their ideas and mechanics, applying them and experimenting can be incredibly worthwhile.”


Jonathan Kohl, Applying Gamification to Software Testing

When people realize that it’s fun to find more bugs and better bugs than the other people on the team, they push each other to try harder, which leads to smarter and better testing, and to everyone learning more about the system. It’s a game, and it can be fun – but it’s serious business too.

Peer reviews for security are a waste of time?

At this year’s RSA conference, one of the panel’s questioned whether software security is a waste of time. A panellist, John Viega, said a few things that I agreed with, and a lot that I didn't. Especially that

“peer reviews for security are a waste of time.”

This statement is wrong on every level.

Everyone should know by now that code reviews find real bugs – even informal, lightweight code reviews.

“Reviews catch more than half of a product’s defects regardless of the domain, level of maturity of the organization, or lifecycle phase during which they were applied”. What We Have Learned About Fighting Defects

Software security vulnerabilities are like other software bugs – you find them through testing or through reviews. If peer code reviews are a good way to find bugs, why would they be a waste of time for finding security bugs?

There are only a few developers anywhere who write security plumbing: authentication and session management, access control, password management, crypto and secrets handling. Or other kinds of plumbing like the data access layer and data encoding and validators that also have security consequences. All of this is the kind of kind of stuff that should be handled in frameworks anyway – if you’re writing this kind of code, you better have a good reason for doing it and you better know what you are doing. It’s obviously tricky and high-risk high-wire work, so unless you’re a team of one, your code and decisions need to be carefully reviewed by somebody else who is at least as smart as you are. If you don’t have anyone on the team who can review your work, then what the hell are you doing trying to write it in the first place?

Everybody else has to be responsible for writing good, defensive application code. Their responsibilities are:

• Make sure their code works – that the logic is correct


• Use the framework properly


• Check input data and return values


• Handle errors and exceptions correctly


• Use safe routines/APIs/libraries


• Be careful with threading and locking and synchronization

A good developer can review this code for security and privacy requirements: making sure that you are masking or encrypting or – even better – not storing PII and secrets, auditing, properly following access control rules. And they can review the logic and workflow, look for race conditions, check data validation, make sure that error handling and exception handling is done right and that you are using frameworks and libraries carefully.

This is what code reviews are for. To look for and find coding problems. If you find these problems – and code reviews are one of the most effective ways of doing this – your code will be safer and more secure. So I don’t get it. Why are peer reviews for security a waste of time?