Category: Code Analysis

Analyzing iOS crash dumps with Xcode

The best way to analyze a crash in an iOS app is if you can reproduce it directly in the iOS simulator in debug mode or on a local device connected to Xcode. Sometimes you have to analyze a crash that happened on a device that you do not have direct access to. Maybe the crash was discovered by a tester who is located in a remote place. In this case the tester must transfer the crash information to the developer and the developer has to import it in Xcode. The iOS and Xcode functionalities for this workflow are a bit hidden, so that the following step-by-step guide can help.

Finding the crash dumps

iOS stores crash dumps for every crash that occured. You can find them in the Settings app in the deeply nested menu hierarchy under Privacy -> Analytics -> Analytics Data.

There you can select the crash dump. If you tap on a crash dump you can see its contents in a JSON format. You can select this text and send it to the developer. Unfortunately there is no “Select all” option, you have to select it manually. It can be quite long because it contains the stack traces of all the threads of the app.

Importing the crash dump in Xcode

To import the crash dump in Xcode you must save it first in a file with the file name extension “.crash”. Then you open the Devices dialog in Xcode via the Window menu:

To import the crash dump you must have at least one device connected to your Mac, otherwise you will find that you can’t proceed to the next step. It can be any iOS device. Select the device to open the device information panel:

Here you find the “View Device Logs” button to open the following Device Logs dialog:

To import the crash dump into this dialog select the “All Logs” tab and drag & drop the “.crash” file into the panel on the left in the dialog.

Initially the stack traces in the crash dump only contain memory addresses as hexadecimal numbers. To resolve these addresses to human readable symbols of the code you have to “re-symbolicate” the log. This functionality is hidden in the context menu of the crash dump:

Now you’re good to go and you should finally be able to find the cause of the crash.

Developer power tools: Big O = big impact

Scalability and performance are often used interchangeable but they are very different. The big O notation helps in talking about scalability.

Scalability and performance are often used interchangeable but they are very different. The resource consumption of a program is its performance. So performance looks at a specific part of a program, say the user activating an action and tries to fix the input and circumstances.
Scalability defines how the resource consumption of a program changes when additional resources are added or the input size increases. So a program can have good performance (it runs fast) but when the load increases it can behavely badly (a bad scalability). Take bubble sort for example. Since the algorithm has low overhead it runs fast with small arrays but when the arrays get bigger the runtime gets worse. It doesn’t scale.
There are many ways to improve scalability here we look at one particular: reducing algorithmic complexity. The time complexity of an algorithm is measured with the big T notation. The T notation describes the time behavior of the algorithm when the input size changes. Say you have an algorithm that takes n objects and needs 4 times as long with an overhead of 2. This would be:

T(n) = 4n + 2

Often the correct numbers are not necessary, you just want to have a magnitude. This is where the big O notation comes into play. It describes the asymptotical upper bound or just the behaviour of the fastest growing part. In the above case this would be:

T(n) = 4n + 2 = O(n)

We call such an algorithm linear because the resource consumption grows linear with the increase in input size. Why does this matter? Let’s look at an example.

Say we have a list of numbers and want to know the highest. How long would this take? A straight forward implementation would look like this:

int max = Integer.MIN_VALUE;
for (int number : list) {
  if (number > max) {
    max = number;
  }
}

So we need if the size of the number list is n we need n compare operations. So our algorithm is linear. What if we have two lists of length n? We just our algorithm twice and compare both maximums so it would be

T(n) = 2n + 1 = O(n)

also linear.
Why does this all matter? If our algorithm runs quadratic, so O(n^2) and our input size is n = 1000, we need a multitude of 1 million operations. If it is linear it just needs a multitude of 1000 operations, much less. Reducing the complexity of an algorithm can be business critical or the difference between getting an instant result or losing a customer because he waited too long.
Time complexity isn’t the only complexity you can measure other resources like space can also be measured with big O. I hope this clears some questions and if you have further ones please feel free and leave a comment.

A mindset for inherited source code

This article outlines a mindset for developers to deal with existing, probably inherited code bases. You’ll have to be an archeologist, a forensicist and a minefield clearer all at once.

One field of expertise our company provides is the continuation of existing software projects. While this sounds very easy to accomplish, in reality, there are a few prerequisites that a software project has to provide to be continuable. The most important one is the source code of the system, obviously. If the source code is accessible (this is a problem more often than you might think!), the biggest hurdle is now the mindset and initial approach of the developers that inherit it.

The mindset

Most developers have a healthy “greenfield” project mindset. There is a list of requirements, so start coding and fulfill them. If the code obstructs the way to your goal, you reshape it in a meaningful manner. The more experience you have with developing software, the better the resulting design and architecture of the code will be. Whether you apply automatic tests to your software (and when) is entirely your decision. In short: You are the master of the code and forge it after your vision. This is a great mindset for projects in the early phases of development. But it will actively hinder you in later phases of your project or in case you inherit foreign code.

For your own late-phase projects and source code written by another team, another mindset provides more value. The “brownfield” metaphor doesn’t describe the mindset exactly. I have three metaphors that describe parts of it for me: You’ll need to be an archeologist, a forensicist (as in “securer of criminal evidence”) and a minefield clearer. If you hear the word archeologist, don’t think of Indiana Jones, but of somebody sitting in the scorching desert, clearing a whole football field from sand with only a shaving brush and his breath. If you think about being a forensicist, don’t think of your typical hero criminalist who rearranges the photos of the crime scene to reveal a hidden hint, but the guy in a white overall who has to take all the photos without disturbing the surrounding (and being disturbed by it). If you think about the minefield clearer: Yes, you are spot on. He has to rely on his work and shouldn’t move too fast in any direction.

The initial approach

This sets the scene for your initial journey inside foreign source code: Don’t touch anything or at least be extra careful, only dust it off in the slightest possible manner. Watch where you step in and don’t get lost. Take a snapshot, mental or written, of anything suspicious you’ll encounter. There will be plenty of temptation to lose focus and instantly improve the code. Don’t fall for it. Remember the forensicist: what would the detective in charge of this case say if you “improved the scenery a bit” to get better photos? This process reminds me so much of a common approach to the game “Minesweeper” that I included the minefield clearer in the analogy. You start somewhere on the field and mark every mine you indirectly identify without ever really revealing them.

Most likely, you don’t find any tests or an issue tracker where you can learn about the development history. With some luck, you’ll have a commit history with meaningful comments. Use the blame view as often as you can. This is your archeological skills at work: Separating layers and layers of code all mingled in one place. A good SCM system can clear up a total mess for you and reveal the author’s intent for it. Without tests, issues and versioning, you cannot distinguish between a problem and a solution, accidental and deliberate complexity or a bug and a feature. Everything could mean something and even be crucial for the whole system or just be useless excess code (so-called “live weight” because the code will be executed, but with no effect in terms of features). To name an example, if you encounter a strange sleep() call (or multiple calls in a row), don’t eliminate or change them! The original author probably “fixed” a nasty bug with it that will come back sooner than you know it.

Walking on broken glass

And this is what you should do: Leave everything in its place, broken, awkward and clumsy, and try to separate your code from “their” code as much as possible. The rationale is to be able to differentiate between “their” mess and “your” mess and make progress on your part without breaking the already existing features. If you cannot wait any longer to clean up some of the existing code, make sure to release into production often and in a timely manner, so you still know what changed if something goes wrong. If possible, try to release two different kinds of new versions:

  • One kind of new version only incorporates refactorings to the existing code. If anything goes wrong or seems suspicious, you can easily bail out and revert to the previous version without losing functionality.
  • The other kind only contains new features, added with as little change to existing code as possible. Hopefully, this release will not break existing behaviour. If it does, you should double-check your assumptions about the code. If reasonably achievable, do not assume anything or at least write an automatic test to validate your assumption.

Personally, I call this approach the “tick-tock” release cycle, modelled after the release cycle of Intel for its CPUs.

Changing gears

A very important aspect of software development is to know when to “change gears” and switch from greenfield to brownfield or from development to maintainance mode. The text above describes the approach with inherited code, where the gear change is externally triggered by transferring the source code to a new team. But in reality, you need to apply most of the practices on your own source code, too. As soon as your system is in “production”, used in the wild and being filled with precious user data, it changes into maintainance mode. You cannot change existing aspects as easily as before.
In his book “Implementation Patterns” (2008), Kent Beck describes the development of frameworks among other topics. One statement is:

While in conventional development reducing complexity to a minimum is a valuable strategy for making the code easy to understand, in framework development it is often more cost-effective to add complexity in order to enhance the framework developer’s ability to improve the framework without breaking client code.
(Chapter 10, page 118)

I not only agree with this statement but think that it partly applies to “conventional development” in maintainance mode, too. Sometimes, the code needs additional complexity to cope with existing structures and data. This is the moment when you’ve inherited your own code.

Clang, The Friendly Compiler

Clang C/C++ compiler can be called The Friendly Compiler, since it makes it much easier to find and understand compile errors and potential bugs in your code. Go use it!

A while back I suggested to make friends with your compiler as a basis for developing high quality code. My focus then was GCC since it was and still is the compiler I use most of the time. Well, turns out that although GCC may be a reasonably good companion on the C/C++ development road, there are better alternatives.

Enter Clang: I had heard about Clang a few times in the past but never gave it a real shot. That changed after I watched Chandler Carruth’s talk at GoingNative 2012.

First of all I was stunned by the quote from Richard Stallman about GCC being deliberatly designed to make it hard to use it in non-free software. I always wondered why IDEs like KDevelop keep reinventing the wheel all the time by implementing their own C/C++ parsers instead of using already existing and free GCC code. This was the answer: THEY SIMPLY COULDN’T!!

One main point of Chandler’s talk was the quality of diagnostic messages of Clang. GCC is a friend that although telling you exactly what’s wrong with your code, it often does it with complicated sentences hidden in walls of text.

Clang on the other hand, tries very hard to comprehend what you really wanted to write, it speaks in much more understandable words and shows you the offending code locations with nice graphics.

You could say that compared to Clang, which is empathic, understanding, pragmatic and always tries to be on the same page with you, GCC comes across more like an arrogant, self-pleasing and I’m-more-intelligent-than-you kinda guy.

Where GCC says: “What? That should be a template instantiation? Guess what, you’re doing WRONG!! “, Clang is more like: “Ok my friend, now let’s sit down together and analyse step-by-step what’s the problem here. I’ll make us tea.”

You’ll find many examples of Clangs nice diagnostic output in Chandler’s talk. Here is another one, as a little teaser:

struct A
{
  std::string _str1;
  std::string _str2;
};

struct AHasher
{
  std::size_t operator() (const A& a)
  {
    return std::tr1::hash()(a._str1) ^
      std::tr1::hash()(a._str2);
  }
};
...
typedef std::tr1::unordered_map<A, int> AMap;
...

What’s wrong with this code? Yes, exactly: the operator in AHasher must be const. Errors with const correctness are typical, easy-to-overlook kind of problems in day-to-day programming. GCCs reaction to something like that is that something discards qualifiers. This may be perfectly right, and after a while you even get used to it. But as you can see with Clang, you can do much better.

The following two screenshots directly compare GCCs and Clangs output compiling the code above. Because there is a template instantiation involved, GCC covers you in its typical wall of text, before it actually tells you what’s wrong (last line).

CLang’s output is much better formated, it shows you the template instantiation steps much more cleanly and in the last line it tells you to the point what is really wrong: …but method is not marked const. Yeah!

 

Bear up against static code analysis

If you ever had the urge to switch off a rule in your static code analysis tool, this article tries to convince you not to do it. By accepting challenges presented by your tools, you become a better developer and clean up your code on the run.

One of the first things we do when we join a team on a new (or existing) project is to set up a whole barrage of static code analysis tools, like Findbugs, Checkstyle or PMD for java (or any other for virtually every language around). Most of these tools spit out tremendous amounts of numbers and violated rules, totally overwhelming the team. But the amount of violations, (nearly) regardless how high it might be, is not the problem. It’s the trend of the violation curve that shows the problem and its solution. If 2000 findbugs violations didn’t kill your project yet, they most likely won’t do it in the future, too. But if for every week of development there are another 50 violations added to the codebase, it will become a major problem, sooner or later.

Visibility is key

So the first step is always to gain visibility, no matter how painful the numbers are. After the initial shock, most teams accept the challenge and begin to resolve issues in their codebase as soon as they appear and slowly decrease the violation count by spending extra minutes with fixing old code. This is the most valuable phase of static code analysis tools: It enables developers to learn from their mistakes (or goofs) without being embarrassed by a colleague. The analysis tool acts like a very strict and nit-picking code review partner, revealing every flaw in the code. A developer that embraces the changes implied by static analysis tools will greatly accelerate his learning.

But then, after the euphoric initial challenges that improve the code without much hassle, there are some violations that seem hard, if not impossible to solve. The developer already sought out his journey to master the tool, he cannot turn around and just leave these violations in the code. Surely, the tool has flaws itself! The analysis brought up a false positive here! This isn’t faulty code at all, it’s just an overly pedantic algorithm without taste for style that doesn’t see the whole picture! Come to think about it, we have to turn off this rule!

Leave your comfort zone

When this stage is reached, the developers have a deep look into the tool’s configuration and adjust every nut and bolt to their immediate skill level. There’s nothing wrong with this approach if you want to stay on your skill level. But you’ll miss a chance to greatly improve your coding skills by allowing the ruleset to be harder than you can cope with now. Over time, you will come up with solutions you now thought are impossible. It’s like fitness training for your coding skills, you should raise the bar every now and then. Unlike fitness training, nobody gets hurt if the numbers of your code analysis show more violations than you can fix up right now. The violations are in the code, if you let them count or not.

Once, a fellow developer complained really loud about a specific rule in a code analysis tool. He was convinced that the rule was pointless and should be switched off. I asked about a specific example where this rule was violated in his code. When reviewing the code, I thought that applying the rule would improve the code’s internal structure (it was a rule dealing with collapsible conditional statements). In the discussion on how to implement the code block without violating the rule, the real problem showed up – my colleague couldn’t think about a solution to the challenge. So we proceeded to implement the code block in a dozen variations, each without breaking the rule. After the initial few attempts that I had to lead program for him, he suddenly came up with even more solutions. It was as if a switch snapped in his head, from “I’m unable to resolve this stupid rule” to “Hey, if we do it this way, we even can get rid of this local variable”.

Embrace challenges

Don’t trick yourself into thinking that just because your analysis tool doesn’t bring up these esoteric violations anymore after you switched off the rules, they are gone. They are still in your code, just hidden and without your awareness. Bear up against your analysis tool and fix every violation it brings you, one after the other. The tools aren’t there to annoy you, they want to help you stay clear of trouble by pointing out the flaws in a clear and precise manner. Once you meet the challenges the tool presents you with, your skill level will increase automatically. And as a side effect, your code becomes cleaner.

Beyond clean code

Even if every analysis tool approves your code as being clean, it can still be improved. You might have a look at Object Calisthenics or similar code training rulesets. They work the same way as the analysis tools, but without the automatic enforcement (yet). The goal is always cleaner code and higher skilled developers.

A tale of scrap metal code – Part III

The third and last part of a series about the analysis of a software product. This part tries to give some rules of thumb on how to avoid failure like in this project.

In the first part of this tale about an examined software project, I described the initial situation and high-level observations about the project. The second part dove into the actual source code and pointed out what’s wrong on this level. This part will summarize everything and give some hints on how to avoid creating scrap metal code.

About the project

If you want to know more about the project, read the first part of this tale. In short, the project looked like a normal Java software, but unfolded into a nightmare, lacking basic requirements like tests, dependency management or continuity.

A summary of what went wrong

In short, the project failed in every respect except being reasonable functional and delivering business value to the customers. I will repeat this sentence soon, but let’s recall the worst parts again. The project had no tests. The project modularization was made redundant by circular dependencies and hardwired paths. No dependency management was in place, neither through the means of a build tool nor by manual means (like jar versions). The code was bloated and overly complex. The application’s data model was a widely distributed network of arbitrary collections with implicit connections via lookup keys. No effort was spent to grasp exception handling or multithreading. The cleverness was rather invested into wildcard usage of java’s reflection API capabilities. And when the cleverness of the developer was challenged, he resorted to code comments instead of making the code more accessible.

How can this be avoided?

First, you need to know exactly what it is you want to avoid. Let me repeat that the project was sold to happily paying customers who gained profit using it. Many software projects fail to deliver this utmost vital aspect of virtually every project. The problem with this project isn’t apparent yet, because it has a presence (and a past). It’s just that it has no future. I want to give some hints how to develop software projects with a future while still delivering business value to the customer.

Avoid the no-future trap

http://www.istockphoto.com/stock-photo-5407438-percent-blocks.phpThe most important thing to make a project future-proof is to restrain yourself from taking shortcuts that pay off now and need to be paid back later. You might want to believe that you don’t need to pay back your technical debts (the official term for these shortcuts) or that they will magically disappear sometimes, but both scenarios are quite unlikely. If your project has any chance to keep being alive over a prolonged amount of time, the technical debts will charge interest.

Of course you can take shortcuts to meet tight deadlines or fit into a small budget. This is called prototyping and it pays off in terms of availability (“time to market”) and scope (“trial version”). Just remember that a prototype isn’t meant for production. You definitely need the extra time and/or budget to fix the intentional shortcomings in the code. You won’t feel the difference right now (hey, it works, what else should it do?), but it will return with compound interest in a few years. The project in this tale was dead after three years. The technical debt had added up beyond being repairable.

Analyzing technical debts

It’s always easy to say that you should “do it right” in the first place. What could the developer for project at hands have done differently to be better off now?

1. Invest in automated tests

When I asked why the project has no tests at all, the developer replied that “it surely would be better to have tests, yet there was no time to write them“. This statement implies that tests take more time to write than they save acting as a guideline and a safety net. And it is probably true for every developer just starting to write tests. You will feel uncomfortable, your tests will be cumbersome and everything will slow down. Until you gain knowledge and experience in writing tests. It is an investment. It will pay off in the future, not right now. If you don’t start now, there will be no future payout. And even better: now your investment, not your debt, will accumulate interest. You might get used to writing tests and start being guided by them. They will mercilessly tell you when your anticipated solution is overly complex. And they will stay around and guard your code long after you forgot about it. Tests are a precaution, not an afterthought.

2. Review and refactor your code

The project has a line count of 80,000 lines of ugly code. I’m fairly confident that it can be reduced to 20,000 lines of code without losing any functionality. The code is written with the lowest possible granularity, with higher concepts lurking everywhere, waiting to be found and exposed. Of course, you cannot write correct, concise and considerate code on your first attempt. This is why you should revisit old code in a recurring manner. If you followed advice number one and brought your tests in place, you can apply every refactoring of the book’s catalog and still be sure that you rather fixed this part instead of breaking it. Constantly reviewing and refactoring your code has the additional advantage of a code base that gets more proficient alongside yourself. There are no “dark regions” (the code to never be read or touched again, because it hurts) if you light them up every now and then. This will additionally slow you down when you start out, but put you on afterburner when you realize that you can rescue any code from rotting by applying the refactoring super-powers that you gained through pratice. It’s an investment again, aiming at midterm return of investment.

3. Refrain from clever solutions

The project of this tale had several aspects that the developer thought were “clever”. The only thing with “clever” is that it’s a swearword in software programming. Remember the clever introduction of wildcard runtime classloading to provide a “plugin mechanism”? Pure poison if you ever wanted your API to be stable and documented, just like a plugin interface should be. Magic numbers throughout your code? Of course you are smart enough to handle this little extra obfuscation. Except when you aren’t. You aren’t sure how exception handling works? Be clever and just “empty catch Exception” everywhere the compiler points you to. In this project, the developer knew this couldn’t be the right solution. Yet, he never reviewed the code when he one day knew how to handle exceptions in a meaningful manner. Let me rest my case by stating that if you write your code as clever as you can handle it, you won’t be able to read it soon, as reading code is harder than writing it.

Summary

Over the course of this tale, you learned a lot about a failed project. In this article, I tried to give you some advice (in the form of three basic rules) on how this failure could probably have been avoided. Of course, the advice isn’t complete. There is much more you could do to improve yourself and your project. Perhaps the best self-training program for developer skills is the Clean Code Developer Initiative (it’s mostly german text yet, so here is an english blog post about it), based upon the book “Clean Code” by Robert C. Martin (Uncle Bob).

Invest in the future of your project and stay clean.

A tale of scrap metal code – Part II

The second part of a series about the analysis of a software product. This part investigates the source code and reveals some ugly practices therein.

In the first part of this tale about an examined software project, I described the initial situation and high-level observations about the project. This part will dive into the actual source code and hopefully reveal some insights. The third and last part will summarize everything and give some hints on how to avoid creating scrap metal code.

About the project

If you want to know more about the project, read the first part of this tale. In short, the project looked like a normal Java software, but unfolded into a nightmare, lacking basic requirements like tests, dependency management or continuity.

About the developer

The developer has a job title as a “senior developer”. He developed the whole project alone and wrote every line of code. From the code, you can tell his initial uncertainty, his learning progress, some adventurous experiments and throughout every file, a general uneasiness with the whole situation. The developer actively abandoned the project after three years of steady development. From what I’ve seen, I wouldn’t call him a “senior” developer at all.

About the code

The code didn’t look very repellent at first sight. But everywhere you looked, there was something to add on the “TODO list”. Let me show you our most prominent findings:

Unassigned constructors

The whole code was littered with constructor calls that don’t store the returned new object. What’s the point in constructing another instance of you throw it away in the next moment, without ever using it? After examining these constructors, it became apparent that they only exist to perform side effects each. The new object is registered with the global data model while it’s still under construction. It was the most dreadful application of the Monostate design pattern I’ve ever seen.

Global data model

Did I just mention the global data model? At the end of the investigation, we found that the whole application state lives in numerous public static arrays, collections or maps. These data fields are accessible from everywhere in the application and altered without any protection against concurrent modifications. These global variables were placed anywhere, without necessarily being semantically associated with their enclosing class. A data model in the sense of some objects being tied together to form an instance net with higher-level structures could not be found. Instead, different lookup structures like index-based arrays and key-based maps are associated by shared keys or obscure indices. The whole arrangement of the different data pieces is implicit, you have to parse the code for every usage. Mind you, these fields are globally accessible.

Manual loop unrolling

Some methods had several hundred lines of the exact same method call over and over again. This is what your compiler does when it unrolls your foreach loops. In this code, the compiler didn’t need to optimize. To add some myth, the n-th call usually had a slight deviation from the pattern without any explanation. Whenever something could easily be repeated, the developer pasted it all over the place. Just by winding up the direct repititions again, the code migth shrink by one quarter in length.

Least possible granularity

Just by skimming over the code, you’d discover plenty of opportunities to extract methods, raising the level of abstraction in the code. The developer chose to stick with the least possible granularity, making each non-trivial code a pain to read. The GUI-related classes, using Swing, were so bloated by trivialities that even a simple dialog with two text fields and one button was represented by a massive amount of code. Sadly, the code was clearly written by hand because of all the mistakes and pattern deviations. If the code had to deal with complex data types like dates, the developer always converted them to primitive data types like int, double or long and performed the necessary logic using basic math operators.

“This code is single-threaded, right?”

Despite being a Java Swing application, the code lacked any strategy to deal with multithreading other than ignoring the fact that at least two threads would access the code. We didn’t follow this investigation path down to its probably bitter end, but we wouldn’t be surprised if the GUI would freeze up occasionally.

“Exceptions don’t happen here”

If you would run a poll on the most popular exception handling strategy for this code, it would be the classic “local catch’n’ignore”. The developer dismissed the fact that exceptions might happen and just carried on. If he was forced to catch an exception, the catch block followed immediately and was empty in most cases. Of course, the only caught exception type was the Exception class itself.

“This might be null“

One recurring pattern of the developer was a constructor call, stored in a local variable and immediate null check. Look at this code sample:

try {
    SomeObject object = new SomeObject();
    if (object != null) {
        object.callMethod();
    }
    [...]
} catch (Exception e) {
}

There is no possibility (that I know of) of object being null directly after the constructor call. If an exception is thrown in the constructor, the next lines won’t be executed. This code pattern was so prevalent in the code that it couldn’t be an accidental leftover of previous code. The accompanying effect were random null checks for used variables and return values.

Destabilized dependencies

If there is one thing that’s capable of derailing every code reader, analysis tool and justified guess, it’s wildcard use of Java’s reflection capabilities. The code for this project incorporates several dozens calls to Class.forName(), basically opening up the application for any code you want to dynamically include. The class names result from obscure string manipulation magic or straight from configuration. It’s like the evil brother of dependency injection.

Himalaya indentation

Looking at the indentation depths of the code, this wasn’t the worst I’ve ever seen. But that doesn’t mean it was pleasant. Like in Uncle Bob’s infamous “A crap odyssey”, you could navigate some classes by whitespace landscape. “Scroll down to the fifth crest, the vast valley afterwards contains the detail you want to know”.

Magic numbers

The code was impregnated with obscure numbers (like 9, 17, 23) and even more bizzare textual constants (like “V_TI_LB_GUE_AB”) that just appeared out of nowhere several times. This got so bad that the original author included lengthy comment sections on top of the biggest methods to list the most prominent numbers alongside their meanings. Converting the numbers to named constants would probably dispel the unicorns, as we all know that unicorns solely live on magic numbers(*). Any other explanation escapes my mind.

(*) On a side note, I call overly complex methods with magic numbers “unicorn traps”, as the unicorns will be attracted by the numbers and then inevitably tangled up in the complexity as they try to make their way out of the mess.

Summary

This was the list of the most dreaded findings in the source code. Given enough time, you can fix all of them. But it will be a long and painful process for the developer and an expensive investment for the stakeholder.

To give you an overall impression of the code quality, here is a picture of the project’s CrapMap. The red rectangles represent code areas (methods) that need improvements (the bigger and brighter, the more work it will take). The green areas are the “okay” areas of the project. Do you see the dark red cluster just right the middle? These are nearly a hundred complex methods with subtle differences waiting to be refactored.

Prospectus

In part three, I’ll try to extract some hints from this project on how to avoid similar code bases. Stay tuned.

A tale of scrap metal code – Part I

The first part of a series about the analysis of a software product. This part investigates some aspects of general importance and works out how they are failed.

This is the beginning of a long tale about an examined software project. It is too long to tell in one blog post, so I cut it in three parts. The first part will describe the initial situation and high-level observations. The second part will dive deep into the actual source code and reveal some insights from there. The third part wraps everything together and gives some hints on how to avoid being examined with such a negative result.

First contact

We made contact with a software product, lets call it “the application”, that was open for adoption. The original author wanted to get rid of it, yet it was a profitable asset. Some circumstances in this tale are altered to conceal and protect the affected parties, but everything else is real, especially on the technical level.

You can imagine the application as being the coded equivalent to a decommissioned aircraft carrier (coincidentally, the british Royal Navy tries to sell their HMS Invincible right now). It’s still impressive and has its price, but it will take effort and time to turn it around. This tale tells you about our journey to estimate the value that is buried in the coded equivalent of old rusted steel, hence the name “scrap metal code” (and this entry’s picture).

Basic fact

Some basic facts about the application: The software product is used by many customers that need it on a daily basis. It is developed in plain Java for at least three years by a single developer. The whole project is partitioned in 6 subprojects with references to each other. There are about 650 classes with a total of 4.5k methods, consisting of 85k lines of code. There are only a dozen third-party dependencies to mostly internal libraries. Each project has an ant build script to create a deployable artifact without IDE interference. On this level, the project seems rather nice and innocent. You’ll soon discover that this isn’t the truth.

Deeper look

Read the last paragraph again and look out for anything that might alert you about the fives major failures that I’m about to describe. In fact, the whole paragraph contains nothing else but a warning. We will look at five aspects of the project in detail: continuity, modularization, size, dependencies and build process. And we won’t discover much to keep us happy. The last paragraph is the upmost happiness you can get from that project.

Feature continuity

You’ve already guessed it: Not a single test. No unit test, zero integration tests and no acceptance test other than manually clicking through the application guided by the user manual (which we only hoped would exist somewhere). No persisted developer documentation other than generated APIDoc, in which the only human-written entries were abbreviated domain specific technical terms. We could also only hope that there is a bug tracker in use or else the whole project history would be documented in a few scrambled commit messages from the SCM (one thing done right!).
The whole project was an equally distributed change risk. The next part will describe some of the inherent design flaws that prohibited changes from having only local effects. Every feature could possibly interact with every other piece of code and would probably do so if you keep trying long enough.
It’s no use ranting about something that isn’t there. Safety measures to ensure the continuity of development on the application just weren’t there. FAIL!

Project modularization

The six modules are mostly independent, but have references to types in other modules (mostly through normal java imports). This would not cause any trouble, if the structure of the references was hierarchical, with one module on top and other modules only referencing moduls “higher” in the hierarchy. Sadly, this isn’t the case, as there is a direct circular dependency between two modules. You can almost see the clear hierarchical approach that got busted on a single incident, ruining the overall architecture. You cannot use Eclipse’s “project dependencies” anymore, but have to manually import “external class folders” for all projects now. The developer has forsaken the clean and well supported approach for a supposable short-term achievement, when he needed class A of module X in the context of module Y and didn’t mind the extra effort to think about a refactoring of the type and package structure. What could have been some clicks in your IDE (or an automatic configuration) will now take some time to figure out where to import which external folder and what to rebuild first because of the cycle. FAIL!

Code size

The project isn’t giantic. Let’s do some math to triangulate our expectations a bit. One developer worked for three years to pour out nearly 90k lines of code (with build scripts and the other stuff included). That’s about 30k lines per year, which is an impressive output. He managed to stuff these lines in 650 classes, so the average class has a line count of 130 lines of code. Doesn’t fit on a screen, but nothing scary yet. If you distribute the code evenly over the methods, it’s 19 lines of code per method (and 7 methods per class). Well, there I get nervous: twenty lines of code in every method of the system is a whole lot of complexity. If a third of them are getter and setter methods, the line count rises to an average of 26 lines per method. I don’t want every constructor i have to use to contain thirty lines of code!

To be sure what code complexity we are talking about, we ran some analysis tools like JDepend or Crap4j. The data from Crap4j is very explicit, as it categorizes each method into “crappy” or “not crappy”, based on complexity and test coverage (not given here). We had over 14 percent crappy methods, in absolute numbers roughly 650 crappy methods. That is one crappy method per class. The default percentage gamut of Crap4j ends at 15 percent, the bar turns red (bad!) over 5 percent. So this code is right at the edge of insanity in terms of accumulated complexity. If you want to know more about this, look forward to the next parts of this series.

Using the CrapMap, we could visualize the numerical data to get an overview if the complexity is restricted to certain parts of the application. You can review the result as a picture here. Every cell represents a method, the green ones are okay while the red ones are not. The cell size represent the actual complexity of the method. As you can see, the “overly complex code syndrome” is typically for virtually all the code. Whenever a method isn’t a getter or setter (the really tiny dark green square cells), it’s mostly too complex. Additional numbers we get from the Crap4j metric are “Crap” and “Crap Load”, stating the amount of “work” necessary to tame a code base. Both values are very high given the class and method count.

All the numbers indicate that the code base is bloated, therefore constantly using the wrong abstraction level. Applying non-local changes to this code will require a lot of effort and discipline from an experienced developer. FAIL!

Third-party dependencies

The project doesn’t use any advanced mechanism of dependency resolution (like maven or ant ivy). All libraries are provided alongside the source code. This isn’t the worst option, given the lack of documentation.
A quick search for “*.jar” retrieves only a dozen files in all six modules. That’s surprisingly less for a project of this size. Further investigation shows some inconvenient facts:

  • Some of these libraries are published under commercial licenses. This cannot always be avoided, but it’s an issue if the project should be adopted.
  • Most libraries provide no version information. At least a manifest entry or an appended version number in the filename would help a lot.
  • Some libraries are included multiple times. They are present for every module on their own, just waiting to get out of sync. With one library, this has already happened. It’s now up to the actual classpath entry order on the user’s machine how this software will behave. The (admittedly non-present) unit tests would not safeguard against the real dependency, but the local version of the library, which could be newer or older.

As there is no documentation about the dependencies, we can only guess about their scope: Maybe the classes are required at compile time but optional at runtime? The best bet is to start with the full set and accept another todo entry on the technical debt list. FAIL!

Build scripts

But wait, for every module, there is a build script. A quick glance shows that there are in fact four build scripts for every module. All of them are very similar with minor differences like which configuration file gets included and what directory to use for a specific fileset. Nothing some build script configuration files couldn’t have handled. Now we have two dozen build scripts that all look suspiciously copy&pasted. Running one reveals the next problem: All these files contain absolute paths, as if the “works on my machine award” was still looking for a winner. When we adjusted the entries, the build went successful. The build script we had to change was a messy collection of copied code snippets (if you want to call ant’s XML dialect “code”). You could tell by the different formatting, naming and solution finding styles. But besides being horribly mangled, the build included code obfuscation and other advanced topics. Applied to the project, it guaranteed that no stacktrace from any user would ever contain useful information for anybody, including the project’s developers. FAIL!

Summary

Lets face the facts: The project behind the application fails on every aspect except delivering value to the current customers. While the latter is the most important ingredient of a successful project, it cannot be the only one and is only sustainable for a short period of time. The project suffers from the lonely superhero syndrome: one programmer knows everything (and can defend every design decision, even the ridiculous ones) and has no incentive to persist this knowledge. And the project will soon suffer from the truck factor: The superhero programmer will not be available anymore soon.

Prospectus

There are a lot of take-away lessons from this project, but I have to delay them until part three. In the next part, we’ll discover the inner mechanics and flaws of the code base.

Combine cobertura with the awesomeness of crap4j

Want the awesomeness of crap4j without running your tests twice in your build? Just combine it with your cobertura data using crapertura.

You may have heard of crap4j when it was still actively developed. Crap4j is a software metric that points you to “crappy” methods in your projects by combining cyclomatic complexity numbers with test coverage data. The rationale is that overly complex code can only be tamed by rigorous testing or it will quickly reduce to an unmaintainable mess – the feared “rotten code” or “crappy code”, as Alberto Savoia and Bob Evans, the creators of crap4j would put it. The crap4j metric soon became our most important number for every project. It’s highly significant, yet easy to grasp and mandates a healthy coding style.

Some enhancements to crap4j

Crap4j got even better when we developed our own custom enhancements to it, like the CrapMap or the crap4j hudson plugin. We have a tool that formats the crap4j data like cobertura’s report, too.

A minor imperfection

The only thing that always bugged me when using crap4j inside our continuous integration build cycle was that at least half the data was already gathered. Cobertura calculates the code coverage of our tests right before crap4j does the same again. Wouldn’t it be great if the result of the first analysis could be re-used for the crap metric to save effort and time?

Different types of coverage

Soon, I learnt that crap4j uses the “path coverage” to combine it with the complexity of a method. This is perfectly reasonable given that the complexity determines the number of different pathes through the method. Cobertura only determines the “line coverage” and “branch coverage”. As it stands, you can’t use the cobertura data for crap4j because they represent different approaches to measure coverage. That’s still true and probably will be for a long time. But the allurement of the shortcut approach was too high for me to resist. I just tried it out one day to see the real difference.

A different metric

So, here it is, our new metric, heavily inspired by crap4j. I just took the line and branch coverage for every method and multiplied them. If you happen to have a perfect coverage (1.0 on both numbers), it stays perfect. If you only have 75% coverage on both numbers, it will result in a “crapertura coverage” of 56,25%. Then I fed this new coverage data into crap4j and compared the result with the original data. Well, it works on my project.

Presenting crapertura

Encouraged by this result, I wrote a complete ant task that acts similar to the original crap4j ant task. You can nearly use it as a drop-in replacement, given that the cobertura XML report file is already present. Here is an example ant call:


<crapertura
coberturaReportFile="/path/to/cobertura/coverage.xml"
targetDirectory="/where/to/place/the/crap4j/report"
classesDirectory="/your/unarchived/project/class/files"
/>

It will output the usual crap4j report files to the given target directory. Please note that even if it looks like crap4j data, it’s a different metric and should be treated as such. Therefore, online comparison of numbers is disabled.

The whole project is published on github. Feel free to browse the code and compile it for yourself. If you want a binary release, you might grab the latest jar from our download server.

The complete usage guide can be found on the github page or inside the project. If you have questions or issues, please use the comment section here.

Conclusion

If crapertura is able to give you nearly the numbers that crap4j gave you is up to your project, really. Our test project contained over 20k methods, but very little crap. The difference between crap4j and crapertura was negligible. Both metrics basically identified the same methods as being crappy. Your mileage may vary, though. If that’s the case, let us know. If your experience is like ours, you’ve just saved some time in your build cycle without sacrificing quality.

FindBugs-driven bughunting in legacy projects

I have been working on a >100k lines legacy project for a while now. We have to juggle customer requests, bug fixes and refactoring so it is hard to improve the quality and employ new techniques or tools while keeping the software running and the clients happy. Initially there were no unit tests and most of the code had a gigantic cyclomatic complexity. Over the course of time we managed to put the system under continuous integration, employed quite some unit tests and analyzed code “hotspots” and our progress with crap4j.

Normally we get bug reports from our userbase or have to test manually to find bugs. A few weeks ago I tried a new approach to bughunting in legacy projects using FindBugs. Many of you surely know this useful tool, so I just want to describe my experiences in that project using FindBugs. Many of the bugs may be in parts of the application which are seldom used or only appear in hard to reproduce circumstances. First a short list of what I encountered and how I dealt with it.

Interesting found bugs in the project

  • There was a calculation using an integer division but returning a double. So the actual computation result was wrong but yet the error would have been hard to catch because people rarely recalculate results of a computer. When writing the test associated to the bugfix I found a StackOverFlowError too!
  • There were quite some null dereferences found, often in contructs like 
     if (s == null && s.length() == 0)

    instead of

    if (s == null || s.length() == 0)

    which could be simplified or rewritten anyway. Sometimes there were possibilities for null dereferences on some paths despite of several null checks in the code.

  • Many performance bugs which may or may not have an effect on overall performance of the system like: new String(), new Integer(12), string concatenation across loops, inefficient usage of java.util.Map.keySet() instead of java.util.Map.entrySet() etc.
  • Some dead stores of local variables and statements without effect which could be thrown away or be corrected to do the intended things.

Things you may want to ignore

There are of course some bugs that you may ignore for now because you know that it is a common pattern in the team and abuse and thus errors are extremely unlikely. I, for example, opted to ignore some dozens of “may expose internal representation” found bugs regarding arrays in interfaces or accessibly via getters because it is a common pattern on the team not to tamper existing arrays as they are seen as immutable by the team members. It would have taken too much time to fix all those without that much of a benefit.

You may opt to ignore the performance bugs too but they are usually easy to fix.

Tips

  • If you have many foundbugs, fix the easy ones to be able to see the important ones more easily.
  • Ignore certain bug categories for now, fix them later, when you stumble upon them.
  • Concentrate on the ones that lead to wrong behaviour and crashes of your application.
  • Try to reproduce the problem with unit test and then fix the code whenever feasible! Tests are great to expose the bug and fix it without unwanted regressions!
  • Many bugs appear in places which need refactoring anyway so here is your chance to catch several flies at once.

Conclusion

With FindBugs you can find common programming errors sprinkled across the whole application in places where you probably would not have looked for years. It can help you to understand some common patterns of your team members and help you all to improve your code quality. Sometimes it even finds some hard to spot errors like the integer computation or null dereferences on certain paths. This is even more true in entangled legacy projects without proper test coverage.