DaedTech

Stories about Software

Jun
17

By

Code Reviews Should Be about Incremental Improvement

Category: The Life of a Programmer Tags: , | 4 Comments

Persuasion tends to happen at the micro-level, in terms of information volume and breadth. For instance, if I explain that I use “var x = 6” instead of “int x = 6” because a productivity add-in squawks if I don’t, you’re likely to be swayed if you find my case persuasive. If, instead, I explain that sometimes I use implicit typing and sometimes I don’t, and then I proceed to list dozens of heuristics, you’re most likely to tune out and, if I’m lucky, process and be swayed by one or two individual bullets that I mention. In these cases the grain of the subject matter is fine, but it works for coarser grained concepts as well: I may sway you on whether or not to write unit tests, onion versus layered architecture, or C# over Java. It isn’t the scope of the topic that matters, but rather the relative narrowness of information and message. Persuasion simply works better when the thesis is clear, and the arguments convincing and too the point.

This bit of human nature is incredibly important to understand when sitting for code reviews, both as a reviewer and reviewee. When I review code and someone starts telling me all in a rush about design patterns that they like and why these six classes communicate over a conceptual bus implementation and why there’s a private method in each class called “PreserveInvariants()” and so on, I’m overloaded with information. Are some of these things good ideas? Probably, I imagine. Are all of them good ideas? I don’t know — slow down. I’m not smart enough to leap instantly into a shared context in which the last several weeks of your design decisions and thinking are jacked into my brain, The Matrix style.

Neo

You’re not going to solve the world’s problems in a given code review. Really, you’re probably not even going to solve all of that many of the code bases’s problems either. But, you can decisively solve or address a few. Much as TDD forces you to focus on one problem at a time, I think well done code reviews follow suit. Aim for a handful of ways in which you can make the code better and upon which all parties agree, and consider that a win for the day. This limtiation to a handful of things to cover will keep the scope of the persuasion narrow enough to be effective, whether it’s the reviewee persuading the reviewer to agree with the choices or the reviewer convincing the reviewee to change things. Or, perhaps it will wind up being a bit of both.

As a reviewer, I generally read through the code, offering first blush impressions and asking questions. “Wow, this method seems pretty big.” “Why did you decide to make that method public rather than private?” There, now there’s one narrow point to address in each case, and we can make our attempts at convincing one another. You can explain by describing where this fits in within the big picture or by whatever means you feel available. This dialog lets us explore your code, style and reasoning together. But if you tell me up front, without prompting, the entire philosophy of your approach and all of your decisions, it will be overload, and you’ll fail to persuade. You’ll succeed only in confusing me. I know it’s tempting to address people’s possible objections before they can raise them (gives the feeling of control), but that’s usually just confusing.

As someone whose code is being reviewed, I let the reviewer process the code under review before the dialog begins. I try to resist the impulse to jump in and explain every facet of my design because it will fall on deaf ears until the code has really been groked, and maybe even after if I’m bombarding the reviewer with information. If I want to provide this information, a writeup is definitely the preferred way to do it since this allows the reviewer to pull the information as desired, rather than having it sprayed like a firehose.

So, code reviews ought to cover a handful of concepts only, to allow for the maximum back and forth of persuasion as necessary. All well and good, but you have a lot of code to cover, right? Well, to address this concern, have the code reviews more frequently or even do semi-regular or regular pairing sessions. If your code only gets reviewed once per quarter, there’s a natural tendency for the review to become a performance evaluation of you as a programmer. But that’s a terrible vehicle for narrow, focused persuasion. If you’re having your code reviewed three times per day, then when the reviewer notices a fourth point of discussion, he’s more likely to say, “meh, three is enough for now — we’ll get this other one next time.”

In the end, code reviews are about persuasion. And effectiveness is really what’s important when trying to persuade someone that your way of thinking makes sense. It pretty much goes without saying that the most important part of being persuasive is making a good case, but I’d argue that a close second is the narrowness of focus on the subject for persuasion. I’m unlikely to persuade you that you should copy every facet of my life, but I might just be able to persuade you pick your battles in code reviews.

May
20

By

How to Use a Code Review to Execute Someone’s Soul

Category: The Life of a Programmer Tags: | 31 Comments

I was having an interesting email discussion with someone not too long ago, and he made an offhand comment about code reviews that I noticed, identified with, and thought was probably a common experience. In essence, he said that he was yet to see them done well and be anything other than depressing experiences. For me, that’s been the case with most, but not all, code review environments. But before throwing the baby out with the bathwater, I’m hoping to write a how to guide for creating dirty bathwater in the hopes that you’ll read it and then not create dirty bathwater. By which, I mean, not give bad code reviews.

I have an odd personality characteristic that compels me to watch or listen to things over and over again if I find them compelling. That is, you might say, “hey, this song’s pretty cool,” and play it for me. If I really take to it, I pity you, because there’s a chance that I’ll play it over and over for like 45 minutes or so. I always feel sort of like a neurotic weirdo, so I try to play it cool and not do it in front of others too much, but, whatever. I’m getting less and less inclined to conceal my weirdness as I get older. I mention this as a segue to explaining the post title.

When I was younger, I watched the Al Pacino movie, “Scent of a Woman,” and I came to be very fond of the Al Pacino character, Frank. He said a lot of very memorable things in that movie, in kind of a gruff, misanthropic, wolfish, but nice-guy-when-you-get-to-know him way. I won’t go through all the quotes here, but I probably could. The reason I could is that I watch things enough times to commit huge swaths of them to memory, and this was the case with this movie. Indeed, this is the reason for the neurotic replay — I don’t just want to experience things; I want to command them and be able to recall and recite them at will later. Anyway, near the end of the movie, Frank gives this extended, loud, angry soliloquy defending his reluctant protege, who is on the precipice of expulsion from school. During the course of this, when talking about the effect that expelling him would have, he says, “you think you’re merely sending this splendid foot-soldier back home to Oregon with his tail between his legs, but I say you are executing his soul!”

What he’s referring to is that the boarding school is on the verge of expelling the protege for taking what he feels to be a principled stand and refusing to “rat out” a classmate in exchange for favorable treatment. The Frank Slade character is saying, in essence, “by punishing this kid for ethical behavior, you’re jading him and altering the course of his life and interactions with others.” And I think that this sentiment has a parallel in our world of programming: I have seen code reviews, things that should be a collaborative, learning experience, subvert group dynamics and result in negativity and hostility being paid forward. So, if you’re interested in a guide on how to do just that and how to turn optimistic, bright eyed developers into angry, cynical people, look no further, because I’m going to tell you.

Nitpick and bikeshed

I think most of us would agree that there’s nothing like spending an hour or two arguing about whether to use implicit typing or not or whether it’s better to trap a null parameter and throw ArgumentNullException or to just let the NullReferenceException happen organically. So, why not make all of your code reviews like that? You can forget about big picture concerns or pragmatic but philosophical discussions such as code readability and ease of maintenance and opt instead to have heated arguments about the little things. Anyone who had previously viewed code reviews as opportunities for learning and collaboration will quickly come to realize that they’re the place for heated disagreements about subjective opinions on details.

Be condescending and derisive

Pointing out a possible logic error is something that would go on in a healthy code review, but why stop there? If pointing out the error drives the point home, adding an “I can’t believe anyone could miss that” will really light a fire under them not to do it again. Since your code reviews are one-way activities, there’s no chance of someone on the other side of the table noticing your mistakes later, so you might as well let ‘er rip with the insults and really dig into their self confidence. They’ll learn to look forward to the day when they can follow in your footsteps and humiliate junior developers from right where you’re sitting.

ExpertBeginner

Make them interminable

If a little code review is good, a 10 hour marathon has to be great. Make sure you go over every line of code, every config file change, every markup tag, and every automated refactoring. Bonus points if you get things wrong in the history and review things that other developers checked in, taking the one in the review to task for code that he didn’t even write. Punish developers for getting their work finished by making them justify every character they’ve added to the code base as if it were a PhD Thesis defense.

Make the code reviews gate-keeper reviews that have to be “passed”

While you’re on the professor-student theme, you might as well go ahead and implement a policy that code has to “pass review.” That way, it’s not a team of professionals reviewing one another’s work and offering critiques, suggestions and insights, but rather a terrified junior developer trying to figure out if she’s good enough to check in code this release. Why not make every sprint/iteration/release just like taking the SATs and waiting for college admissions decisions for all involved? There’s nothing like wondering existentially if you’re good enough at life to have a future to keep the creative juices flowing freely.

Pass your opinions off as facts

Is it your opinion that static methods are better than instance methods? No, of course not. It’s just a fact. At least, that’s how you should approach code reviews. Think of all of your personal tastes in design patterns, coding approaches, style, etc, and then remove qualifying phrases like “I think that,” “I prefer,” or “In my opinion.” So, “I like pre-pending field names with underscores” becomes “you should pre-pend your field names with underscores” and “I find out parameters confusing” becomes “out parameters are bad.” And whatever you do, never back anything up with evidence or citations. When you tell someone that their methods are too long and they ask “what makes them too long,” don’t cite any studies about the correlation between method length and increased defect counts, just see the point about condescension and derision and say something like, “the fact that I’m the senior programmer here and yours are a lot longer than mine.”

Cover things for which checks could be easily automated

Why spend the day on productive tasks when you could sit in a cramped meeting room and pore exhaustively through the code looking to see if anyone committed the grave sin of using camelCase instead of PascalCase on a method. While you’re at it, count the lines in each method to see if they’re too long. Maybe even compute the cyclomatic complexity by hand and then have arguments over whether this method’s is 4 or 5, if you count the default of the case statement. Sure, there are tools that can do any of those things in seconds, but then you lose out on the human touch whereby junior developers are publicly rebuked for their mistakes instead of learning about them quietly and quickly from an automated tool.

Focus only on the negative

It’s amazing how just a little bit of positive feedback can brighten the whole experience, and there should be plenty from which to choose, since examining others’ code always presents an opportunity to see new ways of doing things. So, because of that, you have to be careful to keep it all negative, all the time. Use a whiteboard or a spreadsheet to list people’s mistakes, and the more, the better. Everyone should walk out of code reviews pretty convinced that they chose the wrong line of work and that they can’t hack it. Lights at the end of the tunnel are for cowards.

The Takeaway

I’m hoping it’s pretty obvious that this has been a tongue-in-cheek post, but there’s a serious takeaway here. It’s easy to allow code reviews to become negative, depressing and even political affairs. I’ve heard an awful lot of developers talking about hating them, and I think it’s for a lot of the reasons mentioned here — they’re needlessly adversarial and draconian, and people learn to dread them. I also think that there’s kind of a human nature tendency to “pick on the new kid” and make these things tiered affairs where the council of elders passes judgment and doles out tribal knowledge.

To keep code reviews healthy and beneficial, I believe that a concerted effort is required. I might make a post on this as a separate subject in the future, but some general ideas all revolve around staying positive and respectful. If you see things that could be errors, you don’t need to tell people they’re wrong, usually a “what do you think would happen if I passed null into this method” would suffice because the person will probably say, “oh, I didn’t think of that — I’ll fix it when we’re done here.” Allowing them to solve the problem and propose the improvement is empowering and so, so much better than giving them orders to fix their deficient code.

Look at code reviews not as exams or activities to prove the worth of the code, as-written, but as opportunities for groups of people to work toward better solutions. Instill a collective sense of code ownership instead of different people slamming their visions together and watching sparks fly. Reduce the friction in code reviews by complementing the activity with pair or mob programming. That way, more people are defending the work together than the group picking on a single person, and you also get the benefit of not spending days or weeks building up a defensive attachment to your code.

I have seen studies suggesting that the most effective way to reduce defect counts is not to try really hard or run static analysis tools or even (hold your noses, TDD fans) unit tests. The best way, bang-for-buck, to reduce defect count is through pair inspection. It’s far too valuable a tool simply to toss. We just have to be decent about it and treat one another like human beings.

By the way, if you liked this post and you're new here, check out this page as a good place to start for more content that you might enjoy.

Oct
11

By

In Search of the Perfect Code Review

Category: The Life of a Programmer Tags: , , , , |

Code Reviews

One thing that I tend to contemplate from time to time and have yet to post about is the idea of code reviews and what constitutes a good one. I’ve worked on projects where there was no code review process, a half-hearted review process, an annoying or counter-productive code review process, and a well organized and efficient code review process. It’s really run the gamut. So, I’d like to put pen to paper (finger to keyboard) and flesh out my ideas for what an optimal code review would entail. Before I can do that, however, I think I need to define some terms up front, and then identify some things about code reviews that I view as pitfalls, counter-productive activities or non-useful activities.

What is a Code Review?

According to the wikipedia entry for a code review, there are three basic types: the formal review, pair programming, and the lightweight review. I’ll add a fourth to this for the sake of pure definition, and call that the “automated review”. The automated review would be use of one or more static analysis tools like FX Cop or Style Cop (the wiki article includes someone using tools like this on the code of another developer, but I’m talking strictly automated steps like “you fail the build if you generate Style Cop warnings”). Pair programming is self explanatory for anyone familiar with the concept. Lightweight reviews are more relaxed and informal in the sense that they tend to be asynchronous and probably more of a courtesy. An example of this is where you email someone a source code file and ask for a sanity check.

The formal review is the one with which people are probably most familiar if code review is officially part of the SDLC on the project. This is a review where the developer sits in a room with one or more other people and presents written code. The reviewers go through in detail, looking for potential bugs, mistakes, failures to comply with convention, opportunities for improvement, design issues, etc. In a nutshell, copy-editing of code while the author is present.

What I’ll be discussing here mainly pertains to the formal review.

What I Don’t Like

Here are some things that I think ought to be avoided in a code review process.

1. Failing to have code reviews

This is important in the same way that QC is important. We’re all human and we could all use fresh eyes and sanity checks.

2. Procedural Code Review: The Nitpick

“You made that camel case instead of Pascal case.” “You could be dereferencing null there.” In general, anything that a static analysis tool could tell someone a lot faster and more accurately than a room full of people inspecting code manually. Suresh addresses this idea in a blog post:

I happen to see only “superficial” reviews happening. By superficial, I mean the types where you get review comments like, “You know the documentation for that method doesn’t have the version number”, or “this variable is unused”, etc.

“Superficial” is an excellent description as these things are generally trivial to identify and correct. It is not an effective use of the time of the developer or reviewers anymore than turning off spell check and doing it manually is an effective use of authors’ and editors’ time. There are tools for this — use them!

3. Paranoid Code Review

This is what happens when reviewer(s) go into the review with the notion that the only thing standing between a functioning application and disaster is their keen eye for the mistakes of others. This leads to a drawn out activity in which reviewers try to identify any possible, conceivable mistake that might exist in the code. It’s often easily identified by reviewers scrunching their noses, concentrating heavily, pointing, or tracing code through control flow statements with their fingers on the big screen while the developer twiddles his thumbs.

Again, there’s a tool for this. It’s called the unit test. It’s not flawless and it assumes a decent amount of coverage and competence from the unit tests, but if executed properly, the unit tests will express and prove corner case behavior far better than people on too little sleep staring at a screen and trying to step through the call stack mentally. This mental execution is probably the least reliable possible way of examining code.

4. Pure Gatekeeper Code Review

This is less of an individual property of code review, but more a property of the review process. It’s where you have a person or committee in the department that acts as the Caesar of code, giving thumbs up or thumbs down to anything that anyone submits. Don’t get me wrong — the aim of this makes sense. You want somebody that’s doing a sanity check on the code and someone who more or less has his or her finger on the pulse of everything that’s happening.

The issue that occurs here is a subtle one that results from having the same person or people reviewing all code. Specifically, those people become the gatekeepers and the submitters become less concerned about writing good code and innovating and more principally concerned with figuring out how to please the reviewer(s). Now, if the reviewers are sharp and savvy, this may not be a big deal, though discouraging new ideas and personal growth is always going to have some negative impact. However, if the reviewers are not as sophisticated, this is downright problematic.

This is relatively easily addressed by rotating who performs code reviews or else distinguishing between “suggestion” and “order”. I’ve participated in review processes where both of these mitigating actions were applied, and it’s a help.

5. Tyrant Gatekeeper

In the previous example, I mentioned a hypothetical gatekeeper reviewer or committee with ultimate authority, and this is a subset of that. In this case, the reviewer(s) have ultimate yes/no authority and are very heavy (and possibly even combative or derisive) with the “no” option. Where the previous example might stifle innovation or developer growth, this creates bottlenecks. Not only is it hard to get things approved, but developers will naturally start asking the reviewer(s) what to do at every turn rather than attempting to think for themselves.

In essence, this creates a state of learned helplessness. Developers are more concerned with avoiding negative feedback at the code reviews than learning, doing a good job, or becoming able to make good decisions based on their own experience. As a result, the developers don’t really make any decisions and ask the reviewer(s) what to do at every step, waiting until they have time, if necessary. The review(s) become a bottleneck in the process.

I have not personally witnessed or been subject to this form of code reviews, but I have heard of such a thing and it isn’t difficult to imagine this happening.

6. Discussion Drift

This occurs during a code review when a discussion of the specific implementation gets sidetracked by a more general discussion of the way things ought to be. Perhaps the code is instantiating an object in the constructor, and a reviewer recommends using dependency injection instead. From here, the participants in the review being to discuss how nice it would be if the architecture relied on a IOC framework instead of whatever it is at the moment.

That’s both a valid and an interesting discussion, but it has nothing to do with some developer checking in implementation code within the framework of the existing architecture. Discussions can have merit and still not be germane to the task at hand.

7. Religious Wars

This occurs during a code review in the following context:

Reviewer 1: You didn’t put curly brackets around that one liner following your if statement. Please add them.
Reviewer 2: No, don’t do that. I hate that.
Reviewer 1: No, it’s better. That way if someone changes the code and adds another statement…. etc.
Reviewer 2: Studies have shown….
Review-ee: Uh, guys….

And so on and so forth. Code reviews can easily devolve into this sort of thing over purely or nearly-purely subjective matters. People get very entrenched about their own subjective preferences and feel the need to defend them. We see this from political affiliation to rooting for sports teams. Subjective matters of preference in code are no different. Neither side is likely to convince the other during the scope of the review, but what is quite likely, and probably certain, is that time will be wasted.

If matters like that are part of the coding standards policy on the project, than it’s an open and shut case. If they’re not, they’re better left alone.

So, How Does a Good Code Review Go?

Having explained what I don’t like in a code review, I’ve provided some context for what I do find helpful. I’m going to outline a procedure that is simply an idea. This idea is subject to suggestions for improvement by others, and ongoing refinement from me. Also, this idea is geared toward the gatekeeper scenario. I may make another post on what I perceive to be the most effective method for voluntary(ish), peer-conducted code reviews where the reviewer(s) are not approval gate-keepers.

  1. Developer finishes the pre-defined unit of code and is ready to have it reviewed for promote/commit.
  2. Developer runs static analysis tools (e.g. StyleCop, FXCop, Code Contracts, NDepend, etc) with configured rule-sets, correcting any errors they uncover.
  3. Once no static-check violations are present, developer notifies reviewer(s) of desire for code review.
  4. Reviewers run static analysis tools asynchronously and reject the request if any rules are violated.
  5. Reviewers examine the code for obvious mistakes not caught by static analysis and/or developer unit tests, and write unit tests that expose the deficiency. (Alternatively, they can run something like Pex)
  6. Developer makes reviewer unit tests pass or else convinces reviewer(s) why they don’t need to.
  7. With all unit tests passing, and all parties familiar with the code, a review meeting is setup (meeting can be skipped for smaller/less crucial code deliveries).
  8. Meeting proceeds as follows:
    1. A mediator who is neither developer nor reviewer is asked to attend to keep the meeting focused and on track.
    2. Reviewers point out something praiseworthy about the submitted code (cheesy, perhaps, but important for starting off in the spirit of cooperation)
    3. Reviewers examine code for redundancy (is anything copy/pasted, defined in many places, etc)
    4. Reviewers examine the code for usable API, perhaps by implementing classes in a sandbox, to highlight shortcomings, unintuitive interactions, weird couplings, etc
    5. Reviewers check for architectural consistency — does the class implement a base class that it should, duplicate the function of some other class in the suite, seem to be in the wrong location, etc.
    6. Reviewers perform a dependency analysis — what new dependencies does this introduce? Cyclical, global, temporal, etc. Are these dependencies acceptable?
    7. Reviewers analyze for code smells and anti-patterns.
    8. Reviewers compile a list of suggested changes for the developer.
    9. Meeting adjourned.
  9. Developer makes suggested changes that he agrees with.
  10. Developer appeals changes with which he doesn’t agree. This could involve the reviewer(s) providing “proof”, for example if they say that the developer shouldn’t do something because of X negative consequence, they should demonstrate that consequence somehow. This appeal could be resolved via proof/demonstration or it could go to a third party where the developers and reviewers each state their cases.
  11. Any suggested changes and the results of any appeals are then promoted/committed.

This process naturally addresses (1) and (2) of the things that I don’t like in that you’re having a code review and getting the procedural, easy stuff out of the way offline, prior to meeting. (3) is made more difficult by the fact that the reviewer(s) are given the opportunity to write unit tests that expose the badness about which they might be paranoid. (4) and (5) are addressed by the appeal process and the general concept that changes are suggestions rather than decrees. (6) and (7) are addressed by the mediator who has no skin in the game and will probably have a natural tendency to want to keep things short and sweet.

One drawback I can see to what I’m proposing here is that you could potentially undercut the authority of the reviewer if the person doing the reviews is, say, the most senior or high ranking person. Perhaps people would want that role a bit less if they could be officially second guessed by anyone. However, I think that creates a nice environment where good ideas are valued above all else. If I were in that role myself, I’d welcome the challenge of having to demonstrate/prove ideas that I think are good ones before telling others to adopt them. In the long run, being steered toward that kind of rigor makes you better at your craft. I also think that it might be something of a non-issue, given that people who wind up in these types of leadership and senior roles are often there based on the merit of their work and on successfully “proving” things over and over throughout the course of a career.

Newer Posts »