How to Create a Legacy Code Kata Source Code

From time to time I do a Clean Code Workshop for coworkers or customers. A large part of these workshops deals with legacy code. I always start with the Ugly Trivia Game by J. B. Rainsberger, which actually isn’t by him, but I use his version anyway. While it is a great resource it gets boring after you have reviewed, tested, refactored, tested it some more and reviewed the tests and refactorings. Also: does it really look like the average legacy code we find in our projects? I don’t think so. Real life legacy code has more dependencies and does more really stupid stuff. My favorite so far was a class that loaded stuff from the database in the constructor. Not fun.

Therefor I like to create a little code base for working with legacy code that is very realistic but still small enough so one can handle it in the setting of a workshop and it is actually really simple to do that:

First pick some class that is ugly. Big methods are great. Sometimes you come a cross classes that are super tangled with 2-5 other classes. Those are super awesome. Don’t pick anything to large. A class with 5000 lines just create more work, not more learning opportunities.

Second copy that class (or that small class cluster) in a new project. No dependencies, none of the other stuff it works with. Most likely it wont compile, we gonna fix that in the next step.

Third fix all the compile problems using one of the following approaches:

  • Just add simple libraries like logging or Apache commons to the project.
  • Create missing classes using your IDE, will all the right methods. If the methods look like getter and setter, actually make them getter and setter.
  • If the methods look more complicated just throw a SomethingTerribleIsHappeningHere exception.

This is really fast, works with 98% of all legacy code I came across so far and creates some mean challenges for working with legacy code.

Checklists for the win!

I recently read “The Checklist Manifesto” and boy was it a great read.

First it was fun to read! Checklists sound like the manifestation of boring, right? How about a story of surgeon who takes care of a minor flesh wound, when the blood pressure of the patient suddenly drops to almost zero. Cutting the patient open the surgeon finds the body filled with blood! Now if checklists are deemed appropriate for avoiding stories like this, they suddenly aren’t that boring anymore, right? And the book is full with stories like this. If you imagine stories that you read visually and can’t look at blood, sorry, this book isn’t for you. Everybody else: Go get and read it.

Professionals of various kinds use checklists. Medical doctors, Pilots of course and many more. Yet many people claim they don’t need checklists. They are smart enough to do the right things. Their job is too complicated to be pressed in simple checklists. Really? So the job of a pilot is somehow “simple”? Or that of a surgeon? Of course it isn’t. But still there are many things that are actually rather simple. And it is rather difficult to not forget about those. That’s when checklists help.

Since I finished the book I established two checklists for myself:

One checklist I go through every morning at work. It looks something like this (without the italics, those are just for you so you can make sense out of the few words) and is written on a post-it taped to my monitor:

  • Check mail. Yes I’m proud that sometimes I’m so focused that I don’t check mail at all, so I need it on my checklist
  • Check calendar. To make sure I’m aware of all important appointments today
  • Check logs. Of all servers, so if there is a new problem, I learn about it.
  • Check Jenkins. Mail notification doesn’t work, too much E-mails.
  • Fill out time sheet. In the morning I remember what I did yesterday and I can enter the time I started to work.

The second checklist is for debugging problems with dependency injection. As much as I like the Springframework, from time to time it complains about being unable to instantiate a bean, because a dependency is missing, although the bean is right there in my IDE. For this I created a checklist that lists all the various possible reasons for this problem:

  • What exactly is the problem described in the stacktrace. No bean of matching type? Or no bean of matching name? Or more than one bean?
  • Is the bean that should get found in the correct source path I have a history of putting beans in test instead of main
  • Does the bean that should get found implement the correct interface? Here it is important to check the full name
  • Does it have the correct name?
  • Are the scopes compatible? I’m not sure if this will actually result in the same problem

I store this one in Evernote and will pull it up when I get this kind of problem the next time.

The first checklist already proved highly useful. The second has still to be tested. I just created it when I tried to fix just that kind of problem the last time.

What checklists do you use?

One last word: Of course as software developers we have an advantage. Many things that ask for a checklist can be implement as a program. So don’t replace your automatic build with a checklist please.

Why I Won’t Accept ANY Magic Number

One of the first things I like to establish in a new project is the use of tools like Checkstyle and Findbugs in order to codify some code guidelines and to avoid bugs that can be determined by static code analysis.

Sooner or later with these tools one stumbles over a case where people have the feeling it is going to far. One such case is the check for Magic Numbers of Checkstyle. It will warn about any use of numeric literals except -1, 0, 1 and 2 that aren’t used to define a constant.

Many developers have a problem with this check as one can see from the resulting code. I have seen code like this

  1. private static final int FOUR = 4;

and also

  1. private static final int FOUR = 5;

and my all time favorite (I’m not making this up!):

  1. firstname = rs.getString(1);
  2. lastname = rs.getString(2);
  3. city = rs.getString(2 + 1);
  4. zip = rs.getString(2 + 2);
  5. country = rs.getString(2 + 2 + 1);

But there is a different case where the discussion comes up. It’s with obvious constants like 100 for converting fractions into percent values or 1024 for conversion between bytes and kibibytes. Some argue these aren’t magic numbers (or magic numbers but not as bad) because their meaning is obvious and they won’t change.

I disagree and will vote to make them constants any time. Here is why:

1. The meaning is NOT obvious. What does value * 100 mean? Is a fraction converted to percent? Is a length measured in meters, converted to centimeters? Or something multiplied by a rough approximation of g*g with g being the gravitational acceleration on earth? Or maybe I’m multiplying something with the size of an array, that happens to be 100. Can’t tell. A simple constant with a proper name will fix that.

2. Ok, I do agree, many of these constants won’t change. But the purpose of defining constants (or methods, or classes) is not (only) to enable later change, but to make reading, understanding and reasoning more easy. So the question if a value might change in the future is completely irrelevant.

3. (This one is the argument that I’m missing in most of the discussions) I just don’t want to think about it or have others think about it. I have seen dozens, probably hundreds of instances, where a properly named constant would have helped tremendously understanding a piece of code. And I have seen very few cases where it might have hurt readability and not a single one where it hurt badly. Therefore: just extract a constant and be done with it.

Note: Just because it is a constant doesn’t mean it has to be public, or even a class level field. If it is used only in a single method a local variable is just fine.

Softwaredevelopment, Learning, Qualitymanagement and all things "schauderhaft"