Code Reviews

“Code reviews are the worst! All the code I have to review is terrible, and people always take offense when I point out problems. And being reviewed is even worse – people always think they know better than I, can you believe it? And it’s all such a waste of time!”

It can be difficult to get a team started on code reviews, especially an established team without an established culture of reviews. Developers can be very defensive about their code, and often don’t see the value in code reviews. But the value is undeniable, and good developers will come to appreciate code reviews once they get used to them. Why?
  • Reviews reduce defects. This is the primary purpose of reviews, and they’re very effective.
  • Reviewing code builds familiarity for the reviewer. The reviewer is exposed to code they might not have worked with before, giving them a broader base of knowledge in their own development work.
  • Reviewing code improves developers ability to self-review. The more code you review, the better you get at reviewing code. The better you are at reviewing code, the better you can review your own code before you commit it. The better reviewed code is before it’s committed, the fewer defects are found in peer review, and the faster peer reviews are.
  • Expecting code to be reviewed encourages developers to self-review. Knowing that someone else is going to go over your changes after you submit them encourages you to self-review to save yourself the embarrassment, however slight, of a failed peer review.
  • Peer review improves consistency. When developers submit code without anyone else looking at it, they tend to follow their own styles and practices. As they review more of each others’ code, they will naturally tend to converge on a fairly similar set of styles and practices.
  • Peer review helps to breed a sense of collective ownership. After peer review, code is no longer “his code” or “her code” or “their code” it’s “our code”.
  • In situations where everyone is an architect – which I strongly support – peer review is even more critical. Collective design is only collective if everyone is looking over each other’s shoulders, seeing how problems are being solved, and suggesting possible alternative solutions. It really helps close the gap between a group of individuals and a true development team.
Countless tools exist for performing code reviews; the review handling built into pull requests in Atlassian’s Bitbucket and Stash is excellent, though there are many solutions, from simple things like adding a review step to your ticket workflow and putting review comments in the ticket, to using a dedicated review tool like Crucible or Reviewboard.

Engineers, Hours, Perks, and Pride

This started as a Google+ post about an article on getting top engineering talent and got way too long, so I’m posting here instead.

I wholeheartedly agree that 18-hour days are just not sustainable. It might work for a brand-new startup cranking out an initial release, understaffed and desperate to be first to market. But, at that stage, you can expect the kind of passion and dedication from a small team to put in those hours and give up their lives to build something new.

Once you’ve built it, though, the hours become an issue, and the playpen becomes a nuisance. You can’t expect people to work 18-hour days forever, or even 12-hour days. People far smarter than I have posited that the most productive time an intellectual worker can put in on a regular basis is 4 to 6 hours per day; after that, productivity and effectiveness plummet, and it only gets worse the longer it goes on.

Foosball isn’t a magical sigil protecting engineers from burn-out. Paintball with your coworkers isn’t a substitute for drinks with your friends or a night in with your family. An in-house chef sounds great on paper, until you realize that the only reason they’d need to provide breakfast and dinner is if you’re expected to be there basically every waking moment of your day.

Burn-out isn’t the only concern, either. Engineering is both an art and a science, and like any art, it requires inspiration. Inspiration, in turn, requires experience. The same experience, day-in, day-out – interacting with the same people, in the same place, doing the same things – leaves one’s mind stale, devoid of inspiration. Developers get tunnel-vision, and stop bringing new ideas to the table, because they have no source for them. Thinking outside the box isn’t possible if you haven’t been outside the box in months.

Give your people free coffee. Give them lunch. Give them great benefits. Pay them well. Treat them with dignity and respect. Let them go home and have lives. Let them get the most out of their day, both at work and at home. You’ll keep people longer, and those people will be more productive while they’re there. And you’ll attract more mature engineers, who are more likely to stick around rather than hopping to the next hip startup as soon as the mood strikes them.

There’s a certain pride in being up until sunrise cranking out code. There’s a certain macho attitude, a one-upmanship, a competition to see who worked the longest and got the least sleep and still came back the next morning. I worked from 8am until 4am yesterday, and I’m here, see how tough I am? It’s the geek’s equivalent to fitness nuts talking about their morning 10-mile run. The human ego balloons when given the opportunity to endure self-inflicted tortures.

But I’m inclined to prefer an engineer who takes pride in the output, not the struggle to achieve it. I want someone who is stoked that they achieved so much progress, and still left the office at four in the afternoon. Are they slackers, compared to the guy who stayed another twelve hours, glued to his desk? Not if the output is there. It’s the product that matters, and if the product is good, and gets done in time, then I’d rather have the engineer that can get it done without killing themselves in the process.

“I did this really cool thing! I had to work late into the night, but caffeine is all I need to keep me going. I kept having to put in hacks and work-arounds, but the important thing is that it’s done and it works. I’m a coding GOD!” Your typical young, proud engineer. They’re proud of the battle, not the victory; they’re proud of how difficult it was.

“I did this really cool thing! Because I had set myself up well with past code, it was a breeze. I was amazed how little time it took. I’m a coding GOD!” That’s my kind of developer. That’s pride I can agree with. They’re proud because of how easy it was.

This might sound like an unfair comparison at first, but think about it. When you’re on a 20-hour coding bender, you aren’t writing your best code. You’re frantically trying to write working code, because you’re trying to get it done as fast as you can. Every cut corner, every hack, every workaround makes the next task take that much longer. Long hours breed technical debt, and technical debt slows development, and slow development demands longer hours. It’s a vicious cycle that can be extremely difficult to escape, especially once it’s been institutionalized and turned into a badge of honor.

My Personal Project Workflow/Toolset

I do a lot of side projects, and my personal workflow and tooling is something that’s constantly evolving. Right now, it looks something like this:

  • Prognosticator for tracking features/improvements, measuring the iceberg, and tracking progress
  • WorkFlowy for tracking non-development tasks (the most recent addition to the toolset)
  • Trac for project documentation, and theoretically for defect tracking, though I’ve not been good about entering defects in Trac recently; it doesn’t seem worth the effort on a one-person project, though with multiple people I think it would be a must
  • Trello for cross-cutting all the above and indicating what’s next/in progress/recently completed, and for quickly jotting down ideas/defects. Most of the defect tracking actually goes in here on one-man projects right now. This is a lot of duplication and the main source of waste in my current process.
  • Bitbucket for source control (I also use Atlassian’s excellent SourceTree as a Git/Hg client.)
It’s been working well for me, the only issue I have is duplication between the tools, and failing to consistently use Trac for defect tracking. What keeps me in Trello is how quick and easy it is to add items to it, and the fact that I’m using it as a catch-all – I can put a defect or an idea or a task into it in a couple of seconds; I just have to replicate it to the appropriate place later, which is the problem.
I think the issue boils down to being torn between having a centralized repository for “stuff to be done” (Trello) and having dedicated repositories catered to each type of thing to be done (Prognosticator, Trac, and WorkFlowy); and convenience. Trello is excellent for jotting something down quickly, but lacks the additional specific utility of the other tools for specific purposes.
I think what I’ll end up doing is creating a “whiteboard” list in WorkFlowy, and using that instead of Trello to jot down quick notes when I don’t have the time to use the individual tools; then I can copy from there to the other tools when I need to. That will allow me to cut Trello down to basically being a Kanban board.

Pragmatic Prioritization

The typical release scheduling process works something like this:

  1. Stakeholders build a backlog of features they’d like to see in the product eventually.
  2. The stakeholders decide among themselves the relative priority of the features in the backlog.
  3. The development team estimates the development time for each feature.
  4. The stakeholders set a target feature list and ship date based on the priorities and estimates.
The problem here is primarily in step 2; this step tends to involve a lot of discussion bordering on arguing bordering on in-fighting. Priorities are set at best based on a sense of relative importance, at worst based on emotional attachment. Business value is a vague and nebulous consideration at most.
I propose a new way of looking at feature priorities:

  1. Stakeholders build a backlog of features they’d like to see in the product eventually.
  2. The stakeholders estimate the business value of each feature in the backlog.
  3. The development team estimates the development time for each feature.
  4. The stakeholders set a target feature list and ship date based on the projected return of each feature – i.e., the estimated business value divided by the estimated development time.
This turns a subjective assessment of relative priorities into an objective estimate of business value, which is used to determine a projected return on investment for each feature. This can then be used to objectively prioritize features and schedule releases.
I’ve been using this workflow recently for one of my upcoming projects, and I feel like it’s helped me to more objectively determine feature priorities, and takes a lot of the fuzziness and hand-waving out of the equation.

Shameless self-promotion: Pragmatic prioritization is a feature of my project scheduling and estimation tool, Rogue Prognosticator

Code Patterns as Microevolution

Code patterns abide by survival of the fittest, within a gene pool of the code base. Patterns reproduce through repetition, sometimes with small mutations along the way. Patterns can even mate, after a fashion, by combining them, taking elements of each to form a new whole. This is the natural evolution of source code.

The first step to taming a code base is to realize the importance of assessing fitness and taking control over what patterns are permitted or encouraged to continue to reproduce. Code reviews are your opportunity to thin the herd, to cull the weak, and allow the strong to flourish.


Team meetings, internal discussions, training sessions, and learning investments are then your opportunity to improve both the quality of new patterns and mutations that emerge, as well as the group’s ability to effectively manage the evolution of your source, to correctly identify the weak and the strong, and to have a lasting impact on the overall quality of the product.

If you think about it, the “broken windows” problem could also be viewed as bad genes being allowed to perpetuate. As the bad patterns continue to reproduce, their number grows, and so does their impact on the overall gene pool of your code. Given the opportunity, you want to do everything you can to make sure that it’s the good code that’s continuing to live on, not the bad.

Consider a new developer joining your project. A new developer will look to existing code as an example to learn from, and as a template for their own work on the project, perpetuating the “genes” already established. That being the case, it seems imperative that you make sure those genes are good ones.

They will also bring their own ideas and perspectives to the process, establishing new patterns and mutating existing ones, bringing new blood into the gene pool. This sort of cross-breeding is tremendously helpful to the overall health of the “code population” – but only if the new blood is healthy, which is why strong hiring practices are so critical.

My Take on "Collective Ownership"/"Everyone is an Architect"

I love the idea of “collective ownership” in a development project. I love the idea that in a development team, “everyone is an architect”. My problem is with the cut-and-dried “Agile” definition of these concepts.

What I’ve been reading lately is a definition of “collective ownership” that revolves around the idea of distributing responsibility, primarily in order to lift the focus on finger-pointing and blaming. A defect isn’t “your fault”, it’s “our fault”, and “we need to fix it.” That’s all well and good, but distributing blame isn’t exactly distributing ownership; and ignoring the source of an issue is a blatant mistake.
The latter point first: identifying the source of an issue is important. I see no need for blame, or calling people out, and certainly no point in trying to use defects as a hard metric in performance analysis. However, a development team isn’t a factory; it’s a group of individuals who are constantly continuing their education, and honing their craft, and in that endeavor  they need the help of their peers and managers to identify their weaknesses so they know what to focus on. “Finding the source of an issue” isn’t about placing blame or reprimanding someone, it’s about providing a learning opportunity so that a team member can improve, and the team as a whole can improve through the continuing education of each member.
In regard to distributing ownership, it’s all too rare to see discussion of distributing ownership in a positive way. I see plenty of people writing about eliminating blame, but very few speaking of a team wherein every member looks at the entire code base and says “I wrote that.” And why should they? They didn’t write it alone, so they can’t make that claim. For the product, they can say “I had a hand in that,” surely. But it’s unlikely they feel like they had a hand in the development of every component.
That brings us around to the idea that “everyone is an architect.” In the Agile sense, this is generally taken to mean that every developer is given relatively free rein to architect the component they’re working on at any given moment, without bowing down to The Architect for their product. I like this idea, in a sense – I’m all for every developer doing their own prototyping, their own architecture, learning their own lessons, and writing their own code. Up to a point.
There is a level of architecture that it is necessary for the entire team to agree on. This is where many teams, even Agile teams, tend to fall back on The Architect to keep track of The Big Picture and ensure that All The Pieces Fit Together. This is clearly the opposite of “everyone is an architect”. So where’s the middle ground?
If a project requires some level of architecture that everyone has to agree on – language, platform, database, ORM, package structure, whatever applies to a given situation – then the only way to have everyone be an architect is design by committee. Panning design by committee has become a cliche at this point, but it has its uses, and I feel this is one of them.
In order to achieve collective ownership, you must have everyone be an architect. In order for everyone to be an architect, and feel like they gave their input into The Product as a whole – or at least had the opportunity to do so – you must make architectural decisions into group discussions. People won’t always agree, and that’s where the project manager comes in; as a not-an-architect, they should have no bias and no vested interest in what choices are made, only that some decision is made on each issue that requires consideration. Their only job in architectural discussions is to help the group reach a consensus or, barring that, a firm decision.
This is where things too often break down. A senior developer or two, or maybe a project manager with development experience, become de facto architects. They make the calls and pass down their decrees, and quickly everyone learns that if they have an architecture question, they shouldn’t try to make their own decision, they shouldn’t pose it to the group in a meeting, they should ask The Guy, the architect-pro-tem. Stand-up meetings turn into a doldrum of pointless status updates, and discussion of architecture is left out entirely.
Luckily, every team member can change this. Rather than asking The Guy when a key decision comes up, ask The Group. Even better, throw together a prototype, get some research together, and bring some options with pros and cons to the next stand-up meeting. Every developer can do their part to keep the team involved in architecture, and in ownership, and to slowly shift the culture from having The Architect to having Everyone Is An Architect.

Be Maxwell’s Demon

Source code tends to follow the second law of thermodynamics, with some small differences. In software, as in thermodynamics, systems tend toward entropy: as you continue to develop an application, the source will increase in complexity. In software, as well as in thermodynamics, connected systems tend toward equilibrium: in development, this is known as the “broken windows” theory, and is generally considered to mean that bad code begets bad code. People often discount the fact that good code also begets good code, but this effect is often hidden by the fact that the overall system, as mentioned earlier, tends toward entropy. That means that the effect of broken windows is magnified, and the effect of good examples is diminished.

In thermodynamics, Maxwell’s Demon thought experiment is, in reality, impossible – it is purely a thought experiment. However, in software development, we’re in luck: any developer can play the demon, and should, at every available opportunity.

Maxwell’s demon stands between two connected systems, defeating the second law of thermodynamics by selectively allowing less-energetic particles through only in one direction, and more-energetic particles through only in the other direction, causing the two systems to tend toward opposite ends of the spectrum, rather than naturally tending toward entropy.

By doing peer reviews, you’re doing exactly that; you’re reducing the natural entropy in the system and preventing it from reaching its natural equilibrium by only letting the good code through, and keeping the bad code out. Over time, rather than tending toward a system where all code is average, you tend toward a system where all code is at the lowest end of the entropic spectrum.

Refactoring serves a similar, but more active role; rather than simply “only letting the good code through”, you’re actively seeking out the worse code and bringing it to a level that makes it acceptable to the demon. In effect, you’re reducing the overall entropy of the system.

If you combine these two effects, you can achieve clean, efficient, effective source. If your review process only allows code through that is as good or better than the average, and your refactoring process is constantly improving the average, then your final code will, over time, tend toward excellence.

Without a demon, any project will be on a continuous slide toward greater and greater entropy. If you’re on a development project, and it doesn’t have a demon, it needs one. Why not you?