Archive for category Software Design

The Value of Mandatory Code Reviews

At a previous job we had mandatory code reviews, where every single change to the code was required to be reviewed before being checked in, even if you were fixing a single typo. At other jobs we did not have any real code review policy; you could get one if you wanted, but it took some nagging to get it to happen. The most value I had seen from code reviews was in the environment where they were mandatory.

To understand the value of mandatory code reviews, first we must understand the value of a single code review.

I write code

Yes. Yes, you do.

Why don’t you have an editor then? Writers have editors to help them make sure they are being coherent in their writing, and to catch errors both syntactic and semantic. Their role is to be a critical eye with the goal of making the end product better. Are you being too wordy? Are you writing for your audience? Are you being too clever?

As a programmer, a code review is your way of getting the code you wrote to be edited. In a code review, you are being edited by not only your peers, but your audience as well. Getting a code review is not only like a writer having an editor review their writing, but also getting a trusted fan to read an early copy of their writing to make sure their audience will be able to follow their narrative.

What was with that rifle?

Code is the story of the system, it’s point is to communicate to the reader what the system is about. One of my favorite quotes about programming is from the forward to the first edition of Structure and Interpretation of Computer Programs:

[…] a computer language is not just a way of getting a computer to perform operations but rather that it is a novel formal medium for expressing ideas about methodology. Thus, programs must be written for people to read, and only incidentally for machines to execute.

–Hal Abelson, Jerry Sussman and Julie Sussman
Structure and Interpretation of Computer Programs

Otherwise, why are we programming in higher level languages instead of binary and punchcards? Why do we care about meaningful names, not only for items in the program, but also recurring patterns across multiple codebases. Why? Because higher level concepts and abstractions help us to define a language we can communicate with each other using, so we express ideas and tell a story with more clarity.

“Remove everything that has no relevance to the story. If you say in the first chapter that there is a rifle hanging on the wall, in the second or third chapter it absolutely must go off. If it’s not going to be fired, it shouldn’t be hanging there.”

–Anton Chekhov

Just as in any other form of a story, when we get the narrative of our code wrong, by introducing extraneous concepts, or poorly weaving in threads of thought from other areas of the code, we are introducing a rifle that never goes off. When we do that we risk that we are no longer telling a story, but merely rambling, leaving the reader confused and trying to understand if there is an underlying point to all of it.

What is the story you are trying to tell?

I am pretty sure it is a safe bet to say everyone who has programmed for even a small amount of time has gotten so deep into the problem, they lose track of the bigger picture. A similar piece of code to what you just wrote lives elsewhere; you just copied and pasted the same code five times; you have created conditionals six levels deep; you start mixing different levels of abstraction. All of these are easily done when trying to get your head around a problem and *just* get it working, because you are *this* close to being done.

You have been down amongst the trees for so long, you have lost sight of the forest; the code has become a jumbled mess of ideas that no longer tells a story, and leads you to miss what should be obvious in the code. Too many times on getting code review, no sooner than first words are just leaving my mouth do I realize that I forgot something big, and then had to proceed to apologize for wasting their time, and let them know I would need to get them back once I fix the issue.

Just knowing you are going to walk someone through the change forces you to get back to the big picture and look at the code you just wrote from a different perspective. Even if it is just rubber ducking what you are going to be reviewing before doing the actual review helps you to get your thoughts straight.

If the best way to find what you don’t know is by trying to teach it, shouldn’t you practice teaching your new code first? By practicing teaching your changes, you help yourself to understand the questions that the person you are going to get to review your code might have, or anybody else who looks at the code.

Get out of my head, and into their heart

Being able to empathize with the person who will be doing the code review allows you to get out of your own head and see the code as someone else will see it. Even better than empathizing with that person, is to have sympathy for the person who will be reviewing the code, or making the next modification to the code, and have a genuine concern for how she will feel when it comes to be her turn to update the code.

That is the difference between thinking “the next person who touches this code is going to be pissed” and “the next person who touches this will be pissed, but no one should feel that way about code, so I should make this better”.

Being able to take that perspective allows you to get a new view of the code that you probably didn’t have at the time when you were heads down trying to “just get it to work.” Being able to sympathize with the reviewer, as well as the following people who are going to be interacting with the code, is going to make you want to have the code be just that much clearer. Odds are that you will be one of the following people who will be interacting with the code that was just changed, since you are now the last person who touched that code.

Do you want to be working in a code base that makes you angry?

The Golden Rule

Treat others as you would like to be treated.

Ask yourself “If I have to come back again and be the next person to work on this, am I going to be upset?” You would go back and make it nicer if you answered “Yes”, wouldn’t you? Why should anything be any different if you asked that question on behalf of someone else? Don’t you want your teammates to do the same for you? Or do you want them to just think “I don’t care, I just want to be done with this. It can be Jimmy’s problem when he works on this next”?

Don’t forget to think of your teammates when you are going to get your code reviewed as well. You are busy trying to get your tasks done, and don’t want to have to spend anymore time than necessary going over someone else’s code, right? You would be upset if a teammate isn’t even sure what they did to address the issue; introduced changes that if they thought about it for a moment before calling you over would be obvious that they break other parts of the application; you make it all the way through the code review only to end the review by finding an issue breaking major parts of the application that would be obvious if they took an extra few minutes to run the tests; you spend extra time pointing out all of the ways their code is not matching the style guide as set forth by the team.

Why is your time so much more special than your teammates, that you can’t spend extra time reviewing the changes on your own first, catching as many errors as you can, and making the review as effortless as possible for the her? Isn’t that how you would like to be treated when asked for a code review?

It is not that big of a deal

Making sure the code meets the good style guidelines is not really that big of a deal on a single change, is it? Correct, individually it is not, but when every change ignores that and takes it’s own format, it becomes death by a thousand cuts. While the code reviews themselves don’t force you to be less lazy in the code you write, it does put additional pressure for you to make the code clearer.

If done well, you have a reviewer who will be “nitpicking” your code. They will be looking for missed cases, duplicated code, code that belongs elsewhere, and hopefully anything they can see that they don’t like about the code. Her job, as the reviewer, is to try and find as much as can be improved in relation to the code you have just changed. It could even be code you didn’t touch, because you have now introduced duplication that should have been gotten rid of. It is your job, to set your ego aside, and take all the punches that are dished out on the code review, and to remember that it is not a personal attack, but with the goal to make the software that much better and easier to adapt.

It is also her job as the reviewer to make sure to tell you when she sees things she likes, and when you have just taught her something new. The goal here is to make the code better, and if you can teach the reviewer something that can make the next piece of code they write better, everybody wins.

A rising tide raises all ships

When you get your feedback on the review, the goal is to be able to take things you didn’t think about and integrate them back in to the code to make it better. Even better is when you can integrate feedback on things you didn’t even know about. This sharing of knowledge tends to be infectious as well, be it an text editor tip, command line trick, language functionality, or a part of the codebase.

You share a tip with Sally, Sally thinks that is useful, uses it, and shares it during her next few reviews both as reviewer, and the person being reviewed; Jacob and Billy then each use it and share it with Jenny and Clyde, as well as Butch and Megan, respectively. Soon that tip has spread though the team and is making the code base, or development experience, that much better.

Information want to be free and code reviews help disseminate information to as many people as possible. The more you can disseminate the information, the better the odds of someone having critical feedback on it becomes.

How Bazaar

“Given a large enough beta-tester and co-developer base, almost every problem will be characterized quickly and the fix will be obvious to someone.”

–Eric S. Raymond

The above is Linus’ Law as formulated by Eric S. Raymond in The Cathedral and the Bazaar. While most workplace projects don’t operate at the scale of large successful open source software, there are likely people on your team that have some sacred knowledge about that system, or know the lore behind it at least. A code review helps to spread that knowledge outside of yourself and increase the odds that the other person will know about the changes you just made.

Also by getting different people to do your code reviews instead of always just one person, you help spread that knowledge out, so that even if that one person somehow never finds out about these changes, you are not the only person who knows about this change now.

You sure are Extreme

But I pair program all day, so I have a constant code review done, right? I agree that pair programming turns the code review knob to 11 to paraphrase Nigel Tufnel, I am not convinced a thorough code review would still not provide useful feedback. While I haven’t done pair programming as rigorously, or religiously, as others, and do find pair programming very useful, I have found the benefits of getting a different pair of eyes on the code still hold even on code generated from pairing.

If anybody has any feedback about the effectiveness of code reviews on code that was produced using pair programming done rigorously, I would love to get your understanding of how much of a benefit you have seen, or not seen.

Sign on the dotted line

“I got someone to review the code, time to check in!”

Sadly no. I found it works best to say the code is not ready to be checked in to the target branch until all issues raised in the code review have been addressed and signed off by the reviewer. I know, I know, I can hear you saying: “I can’t fix this thing that they want cleaned up because it will break something else.”

All issues have to be *addressed*, and signed off by the reviewer, but they do not all have to be *fixed*. If making a change to fix an issue that the reviewer expressed concern about will cause more bugs, or a much larger change, then that needs to be communicated with the reviewer and they need to understand the implications of a change they are suggesting. If you can’t make a change they are suggesting, create the open dialog and inform them why, if they then agree, you have addressed the issue satisfactorily and can move on.

“But they won’t give and are just being stubborn and want to get their way…”

I am making an assumption that we are operating under collective code ownership. Assuming that is the case, if I get a code review from someone I need to remember that it is their code too, they have a stake in this code base and that everyone is on the same team, and the team should have the goal about making things better. Don’t forget that somebody took their time to review your code, and work to understand why they are making the suggestions they are. If one person in the review feels the other is truly being unreasonable, it never hurts to have an impartial party help settle decisions.

Once the issues have been addressed, and everyone agrees that the code will is the best it can be given the current constraints, then you are allowed to get it checked in and merged into the master/main branch. I would also suggest documenting in the change somewhere the people who reviewed the code.

Thanks for all the fish

One final note on the code review process, and that is to thank the person, or people, who reviewed the code for their time and input, especially if they volunteered to do the code review on their own. They took the time to give you feedback on the code you wrote, so it is always nice to recognize that they didn’t have to be the one to do that, and thank them for their feedback and comments.

10 REVIEW CODE; 20 GOTO 10;

While the value a single well done code review provides is independent of how many other code reviews are done, the total value across all code reviews becomes compounded the more code reviews are done. As long as the software keeps changing, the natural state tends to move towards complexity, and only with care and vigilance, do the changes to a software system improve the state of the software.

When code reviews are not mandatory, those who don’t recognize the value of code reviews, either don’t ask for them, or if they do, are just looking to get a checkmark sign off that they followed the suggestion of having a code review. If you have a team where a good portion of the members see the value of code reviews, then they can help enforce the mandatory nature by mentoring and guiding people on what a good code review consists of.

By having a team decide that mandatory code reviews are to be done on all changes, the team declares as an group that they value the result of well done code reviews, and don’t want people to take short cuts in their review, but to treat code reviews with diligence because they are an important part of the development process. Instilling this culture allows people to hold each other to that standard, even when the team has some developers who don’t like code reviews or don’t believe that code reviews are valuable.

Each well done code review that happens is like getting a small dividend on your changes, as a well done code review helps to make sure the code that is being checked in for a change comes out clean. By their nature, code reviews help to make sure that each change that goes into the system is following The Boy Scout rule. For those who aren’t familiar, The Boy Scouts of America have a rule: “Leave the campground cleaner than you found it.”

When applied to the code, and ensured with mandatory code reviews, I have found this one of the best ways to ensure any technical debt that has been accrued gets payments made against it. Mandatory code reviews when backed by the team have a drastic benefit on the code: every change that is being made gets boy-scouted, and those areas with the most churn are going to reap the most benefits, as they are going to get cleaned up quicker.

Having mandatory code reviews, done well, on every change, ensures that the system is always working towards a state of decreased entropy. Mandatory code reviews provide the framework to self-reinforce on your team that you are going well, and as Uncle Bob Martin so frequently points out, that is they only way to go fast.

I would love to know your experiences with code reviews, so comment below and let me know how code reviews have worked for you, positive or negative.

–Proctor

Leave a comment

Support for Arrays in Constructors using Castle Windsor

As I mentioned in my post Aspect Oriented Timing in C# with Castle Windsor, we are using Castle Windsor as our Inversion of Container on our current project.

In the process of adding a couple of new features, there were places where the new code had to fit in to existing places where there were a longer (more than simply just a single if/else) set of conditionals. To address the changes that I needed to make, I added another conditional to go with the existing ones. This change was in the mood of make it work, knowing I would come back after it worked to make it right, and that this change would allow me to confirm the smell I was “sure” I was smelling.

One I had it working, I came back to make it right. The route I wanted to go was to emulate the refactoring Replace Conditional with Polymorphism by creating a “handler” abstraction, and pulling the logic for each of the conditionals into a separate implementation of the handler.

My new interface was defined as:

public interface INewHandler
{
  bool CanHandle(ISomeObject obj);
  void Handle(ISomeObject obj);
}

I then moved out the logic in each of the conditionals, as well as the conditionals themselves into new handlers. The conditionals were moved into the CanHandle method, and the body of the conditional was moved to the HandleMethod of the implementation.

With these defined, it became just a matter of using a LINQ query to find either the first, or all items that could handle the object, and then just call the Handle method on the ones that matched the filter.

injectedHandlers.
    Where(h => h.CanHandle(someObj)).
    ForEach(h => h.Handle(someObj));

After having this, all we need now is the ability to have all of the implementations injected into the constructor. This gets us to the title of this post, about having arrays injected into constructors. First we have to configure the container to add a new ArrayResolver as a subresolver.

container.Kernel.Resolver.AddSubResolver(new ArrayResolver(container.Kernel));

After this, I want to pick up all implementations of the INewHandler in all assemblies.

container.Register(
    AllTypes.FromAssemblyInDirectory(new AssemblyFilter("bin")).
    BasedOn>INewHandler>().
    WithService.Base().
    Configure(c => c.LifeStyle.PerWebRequest)
);

After adding these two to the registration of Castle Windsor, I now get all implementations injected into my constructor when I declare a constructor parameter that is a type of INewHandler[].

Hope someone else can find this helpful.

–Proctor

, , , ,

Leave a comment

Aspect Oriented Timing in C# with Castle Windsor

I was making some refurbishments on some reporting code in our application that used EF and was suffering from the Select N+1 problem. If truth, it was much worse, as it was an Select N+1 problem up to 6 levels deep depending on where the report was run from.

I was changing the code to use a denormalized view from the database, and then run a SQL Query using Entity Framework. When doing this I was asked to get the timings of the report, both against the new way, and the existing way.

As this is incidental to what I was really trying to do, I did not want to litter timing code, and logging mechanisms into classes that already existed. This smelled of Aspect Oriented Programming (AOP). While I had not done anything using AOP before, I knew that it was great for cross-cutting concerns like logging, timings, etc. Having been digging into Clojure and LISP recently, this also seemed like cases of the :before, :after and :around methods in Common LISP, or the similar behavior in Eiffel as pointed out in Bertrand Meyer’s Object Oriented Software Construction, not to mention the time function in Clojure which is a function whose single concern is simply the to manage capturing the timing a function passed into it. My hope was to simplify, or un-complect, the code, and keep those concerns separate.

In our project, we have Castle Windsor setup as the IOC container, and Windsor supports a type of Aspect Oriented Programming using something called Interceptors. I found documentation on setting it up on a post by Ayende, and one Andre Loker. The issue was some of the places I wanted to setup the capturing of the timings were in different areas than where the handlers were registered for Windsor.

After some hunting around, I managed to come up with being able to add an interceptor to an already registered component by using the following line of code, where the IReportingService is the class I want to time the method calls around, and the LogTimingInterceptor is the class that captures the timing of the method call and sends it to the logger:

container.Kernel.GetHandler(typeof(IReportingService)).ComponentModel.Interceptors.Add(new InterceptorReference(typeof(LogTimingInterceptor)));

Hope someone else can find this useful,
–Proctor

, , , , , ,

2 Comments

John Backus on the Assignment Statement

The assignment statement is the von Neumann bottle-neck of programming languages and keeps us thinking in word-at-a-time terms in much the same way the computer’s bottleneck does.

John Backus, 1977
ACM Turing Award Lecture,
Communications of the ACM
August 1978, Volume 2, Number 8

, , , , ,

Leave a comment