Category: Programming Language

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.

Groovy isn’t a superset of Java

Groovy is Java with sugar, right?

Coming from Java to Groovy and seeing that Groovy looks like Java with sugar, you are tempted to write code like this:

  private String take(List list) {
    return 'a list'
  }

  private String take(String s) {
    return 'a string'
  }

But when you call this method take with null you get strange results:

  public void testDispatch() {
    String s = null
    assertEquals('a string', take(s)) // fails!
  }

It fails because Groovy does not use the declared types. Since it is a dynamic typed language it uses the runtime type which is NullObject and calls the first found method!
So when using your old Java style to write code in Groovy beware that you are writing in a dynamic environment!
Lesson learned: learn the language, don’t assume it behaves in the same way like a language you know even when the syntax looks (almost) the same.

Active Object with POCO’s Active Methods

POCO’s ActiveMethods require minimal additional code to implement the Active Object design pattern

Active Object is a well known design pattern for synchronizing access to an object and/or resource. The basic idea is to separate method invocation from method execution which is done in a dedicated thread.

Instead of using the objects interface directly, a client of an Active Object uses some kind of  proxy which enqueues a so-called Method Request for later execution. The proxy finishes immediately and returns to the client some sort of callback, or variable, by which the client can receive the result. These intermeditate result variables are also known as Futures.

As always, there are lots of ways to implement this pattern. For example, if you had an interface like this

class MyObject
{
  public:
    int doStuff(const std::string& param) =0;
    std::string doSomeOtherThing(int i) =0;
};

applying a straight forward implementation, you would first transform this into a proxy and method request classes:

class MyObjectProxy
{
  public:
    MyObjectProxy(MyObject* theObject);
    // proxy methods
    Future<int> doStuff(const std::string& param);
    Future<std::string> doSomeOtherThing(int i);
  private:
    MyObject * _myObject;
};

class MethodRequest_DoStuff :
  public AbstractMethodRequest
{
  public:
    MethodRequest_DoStuff(const std::string& param);
    // all method request classes must implement execute()
    virtual void execute(MyObject* theObject);

  private:
    const std::string _param;
};

… and so on (for more details see this basic paper by Douglas C. Schmidt, or read chapter Concurrency Patterns in POSA2).

It’s easy to see that this implementation produces a lot of boilerplate code. To solve this, you could either cook up some code generation, or look for some language support to reduce the amount of characters you have to type. In C++, some sort of template solution can be the way to go, or…

Introducing Active Methods

With class ActiveMethod together with support classes ActiveDispatcher and ActiveResult the POCO C++ libraries provide very simple and elegant building blocks for implementing  the Active Object pattern.

ActiveMethod:  this is the core piece. When called, an ActiveMethod executes in its own thread.

ActiveResult: this is what I referred to earlier as a Future. Instances of ActiveResult are used to pass the result of an ActiveMethod call back to the client.

ActiveDispatcher: if you only use ActiveMethods, every ActiveMethod thread can execute in parallel.  With ActiveDispatcher as base class, ActiveMethod calls are serialized, thus implementing real™ Active Object behaviour.

Here my earlier example using ActiveMethods:

class MyObject
{
  public:
    // ActiveMethods are initialized in the ctors
    // initializer list
    MyObject()
      : doStuff(this, &MyObject::doStuffImpl),
        doSomeOtherThing(this, &MyObject::doSomeOtherThingImpl)
    {}

    ActiveMethod<int, std::string, MyObject> doStuff;
    ActiveMethod<std::string, int, MyObject> doSomeOtherThing;
  private:
    int doStuffImpl(const std::string& param);
    std::string doSomeOtherThingImpl(int i);
};

This is used as follows:

MyObject myObject;
ActiveResult<std::string> result = myObject.doSomeOtherThing(42);
...
result.wait();
std::cout << result.data() << std::endl;

This solution requires minimal amounts of additional code to transform your lame and boring normal object into a full-fledged Active Object. The only downside is that Active Methods currently can only have one parameter. If you need more, you have to use tuples or parameter objects.

Have fun!

Avoid switch! Use enum!

Recently I was about to refactor some code Crap4j pointed me to. When I realized most of that code was some kind of switch-case or if-else-cascade, I remembered Daniel´s post and decided to obey those four rules.
This post is supposed to give some inspiration on how to get rid of code like:

switch (value) {
  case SOME_CONSTANT:
    //do something
    break;
  case SOME_OTHER_CONSTANT:
    //do something else
    break;
  ...
  default:
    //do something totally different
    break;
}

or an equivalent if-else-cascade.

In a first step, let’s assume the constants used above are some kind of enum you created. For example:

public enum Status {
  ACTIVE,
  INACTIVE,
  UNKNOWN;
}

the switch-case would then most probably look like:

switch (getState()) {
  case INACTIVE:
    //do something
    break;
  case ACTIVE:
    //do something else
    break;
  case UNKNOWN:
    //do something totally different
    break;
}

In this case you don’t need the switch-case at all. Instead, you can tell the enum to do all the work:

public enum Status {
  INACTIVE {
    public void doSomething() {
      //do something
    }
  },
  ACTIVE {
    public void doSomething() {
      //do something else
    }
  },
  UNKNOWN {
    public void doSomething() {
      //do something totally different
    }
  };

  public abstract void doSomething();
}

The switch-case then shrinks to:

getState().doSomething();

But what if the constants are defined by some third-party code? Let’s adapt the example above to match this scenario:

public static final int INACTIVE = 0;
public static final int ACTIVE = 1;
public static final int UNKNOWN = 2;

Which would result in a switch-case very similar to the one above and again, you don’t need it. All you need to do is:

Status.values()[getState()].doSomething();

Regarding this case there is a small stumbling block, which you have to pay attention to. Enum.values() returns an Array containing the elements in the order they are defined, so make sure that order accords to the one of the constants. Furthermore ensure that you do not run into an ArrayOutOfBoundsException. Hint: Time to add a test.

There is yet another case that may occur. Let’s pretend you encounter some constants that aren’t as nicely ordered as the ones above:

public static final int INACTIVE = 4;
public static final int ACTIVE = 7;
public static final int UNKNOWN = 12;

To cover this case, you need to alter the enum to look something like:

public enum Status {
  INACTIVE(4),
  ACTIVE(7),
  UNKNOWN(12);

  private int state;

  public static Status getStatusFor(int desired) {
    for (Status status : values()) {
      if (desired == status.state) {
        return status;
      }
    }
    //perform error handling here
    //e.g. throw an exception or return UNKNOWN
  }
}

Even though this introduces an if (uh oh, didn’t obey rule #4), it still looks much more appealing to me than a switch-case or if-else-cascade would. Hint: Time to add another test.

How do you feel about this technique? Got good or bad experiences using it?

Is Groovy++ already doomed?

<disclaimer>I really like Groovy and other cool languages like Scala, Fantom, Gosu or Clojure targetting the JVM.</disclaimer>

I know the title is a bit provocative but I want to express a concern regarding Groovy++. In my perception most people think of Groovy++ as an extension for Groovy which trades dynamic dispatching for static typing and dispatching yielding performance. So you can just look for hot spots in your code and resolve them with some annotations. Sounds nice, doesn’t it?.

That seems to be the promise of Groovy++ but it isn’t. Alex Tkachman, the founder of the Groovy++ project states this clearly in this comment to an issue with Groovy++: “100% compatibility with regular Groovy is nice when possible and we do our best to keep it but it is not a must.”.

Imho the mentioned issue together with this statement reduces the target audience to a few people who think of Groovy++ as a better Java, not a faster and type-safe Groovy where needed. I do not think there are too many people thinking that way. I think wide adoption of such a Groovy++ will not happen given the alternatives mentioned in the disclaimer above and Groovy itself. I hope they will strive for 100% compatibility with Groovy…

Shrink your dependency list with POCO

POCO is a nice set of C++ libraries which provides elegant solutions for day-to-day tasks.

When you write C++ applications of any sort you are very likely to need support libraries in addition to what comes with C++ (which is not much, btw). Of course, this holds true for any other language. But with Java and its rich JDK for example this need is not so imminent.

Starting at the very beginning, let’s see how fast the need for support arises.

int main(int argc, char** argv)
{
// parsing command line arguments
...

How to parse those command line arguments in a simple and easy way? How about a little help output when the program is called with -h or –help? Ok, we got boost::program_options for this.

Going further in your program you may want to have some sort of logging capability. Unfortunately, as of boost version 1.45 there is nothing to be found there. So you add a nice logging library.

And so on.

But wait! You don’t want to depend on too many 3rd party libraries because, among other things, they add deployment complexity.

Not even Qt, as one of the major players in the C++ framework world, provides solutions to both previous examples. As of version 4.7, no logging and not much support with command line arguments. And you end-up having to use QString, one of many non-std::string classes in C++ frameworks, which can get annoying at times (of course there are reasons why those exist).

I could go on with the list of smaller or larger concerns for which you either roll your own implementation or include yet another library in your project.

Instead I would like to point you to POCO, a nice set of C++ libraries which provide easy solutions for many basic and/or advanced day-to-day tasks. From their website:

Modern, powerful open source C++ class libraries and frameworks for building network- and internet-based applications that run on desktop, server and embedded systems

Besides very basic stuff like logging, date/time handling, threads, memory management, UTF-8, etc. they also provide lots of higher level classes for things like SMTP, POP3, SQL database access and HTTP. They even have a so called C++ Server Page Compiler which is basically something like JSP or Active Server Pages.

And they have no own string class! Yay! Instead they provide lots of functions classes and streams to do string manuipulation on good old std::string.

One thing I like most about POCO, though, is its clean, well-documented and apparently very high quality code. Although it is not overly functional or template-heavy, like you see it in in boost very often, it still provides elegant solutions.

Check it out and shrink your dependency list.

Diving into Hibernate’s Query Cache behaviour

Hibernate is a very sophisticated OR-Mapper and as such has some overhead for certain usage patterns or raw queries. Through proper usage of caches (hibernates featured a L1, L2 cache and a query cache) you can get both performance and convenience if everything fits together. When trying to get more of our persistence layer we performed some tests with the query cache to be able to decide if it is worth using for us. We were puzzled by the behaviour in our test case: Despite everything configured properly we never had any cache hits into our query cache using the following query-sequence:

  1. Transaction start
  2. Execute query
  3. Update a table touched by query
  4. Execute query
  5. Execute query
  6. Transaction end

We would expect that step 5 would be a cache hit but in our case it was not. So we dived into the source of the used hibernate release (the 3.3.1 bundled with grails 1.3.5) and browsed the hibernate issue tracker. We found the issue and correlated it to the issues HHH-3339 and HHH-5210. Since the fix was simpler than upgrading grails to a new hibernate release we fixed the issue and replaced the jar in our environment. So far, so good, but in our test step 5 still refused to produce a cache hit. Using the debugger strangely enough provided us a cache hit when analyzing the state of the cache and everything. After some more brooding and some println()'s and sleep()‘s we found the reason for the observed behaviour in the UpdateTimestampsCache (yes, yet another cache!):

	public synchronized void preinvalidate(Serializable[] spaces) throws CacheException {
		//TODO: to handle concurrent writes correctly, this should return a Lock to the client
		Long ts = new Long( region.nextTimestamp() + region.getTimeout() );
		for ( int i=0; i
			if ( log.isDebugEnabled() ) {
				log.debug( "Pre-invalidating space [" + spaces[i] + "]" );
			}
			//put() has nowait semantics, is this really appropriate?
			//note that it needs to be async replication, never local or sync
			region.put( spaces[i], ts );
		}
		//TODO: return new Lock(ts);
	}

The innocently looking statement region.nextTimestamp() + region.getTimeout() essentially means that the query cache for a certain “region” (e.g. a table in simple cases) is “invalid” (read: disabled) for some “timeout” period or until the end of the transaction. This period defaults to 60 seconds (yes, one minute!) and renders the query cache useless within a transaction. For many use cases this may not be a problem but our write heavy application really suffers because it works on very few different tables and thus query caching has no effect. We are still discussing ways to leverage hibernates caches to improve the performance of our app.

Bug hunting fun with std::sort

Small errors in custom comparison functions used with std::sort can lead to hard-to-find bugs.

The other day I came across a nice little C++-shoot-yourself-in-the-foot at one of our customers. Let’s see how fast you can spot the problem. The following code crashes with segmentation fault sometime, somewhere in the sort call (line 31).

#include <iostream>
#include <vector>
#include <boost/shared_ptr.hpp>
#include <boost/bind.hpp>

using namespace std;
using namespace boost;

enum SORT_ORDER
{
  SORT_ORDER_ASCENDING,
  SORT_ORDER_DESCENDING
};

bool compareValues(const std::string& valueLeft,
                   const std::string& valueRight,
                   SORT_ORDER order)
{
  const bool compareResult = (valueLeft < valueRight);
  if (order == SORT_ORDER_DESCENDING) {
    return !compareResult;
  }
  return compareResult;
}

int main(int argc, char *argv[])
{
  std::vector<std::string> strValues(300);
  std::fill(strValues.begin(), strValues.end(),
            "Hallo");
  std::sort(strValues.begin(), strValues.end(),
            bind(compareValues, _1, _2, SORT_ORDER_DESCENDING));
  return EXIT_SUCCESS;
}

Any ideas? The tricky thing about this bug is that the stacktrace output in the debugger gives absolutely no hint at all about its cause. And this is a simplified version of the real code which has to sort boost::shared_ptrs instead of strings. Believe me, you don’t want to see that stacktrace. Because of the use of boost::bind together with boost::shared_ptrs it looks, well, let’s say intimidating.

Still no idea?

I’ll give you a hint. If the SORT_ORDER is set to SORT_ORDER_ASCENDING everything is fine. …

Ok, the problem is that std::sort algorithm must be given a comparison function (object) that defines a strict weak ordering on the elements that are to be sorted. In other words the comparison function object must implement the ‘<‘ (less than) relationship on the elements.

Unfortunately, lines 20 to 22 break this ordering when SORT_ORDER_DESCENDING is given. The initial idea of this code was that, well, if compareResult gets returned on ascending sort order, lets just return the negation of it when the “negation” of acscending order is requested. This, of course, destroys the strict weak ordering requirement because whenever valueLeft == valueRight, the function returns true, meaning instead that valueLeft < valueRight. And this somehow wreaks havoc inside std::sort.

A better version of the function could be:

...
bool compareValues(const std::string& valueLeft,
                   const std::string& valueRight,
                   SORT_ORDER order)
{
  // solution: return false independent of sort order
  // whenever valueLeft == valueRight
  if (valueLeft == valueRight) {
    return false;
  }
  const bool compareResult = (valueLeft < valueRight);
  if (order == SORT_ORDER_DESCENDING) {
    return !compareResult;
  }
  return compareResult;
}
...

The really annoying thing about this whole issue is that std::sort just randomly crashes with a stack trace that shows nothing but some weird memory corruption going on. After the initial shock, this sends you down the complete wrong bug hunting road where you start looking for spots where memory could be overwritten or the like.

So beware of custom comparison functions or function objects. They might look innocent and easy, but they can give you lot’s of headaches.

Statement against public fields in Java

Every once in a while I talk to people about coding style and sooner or later there is discussion about public fields and getters/setters in Java. I would like to elaborate my opinion on this issue in addition to other quite well balanced articles to a broader audience.

First I want to differentiate properties of a class from other fields/member variables. Properties are fields, whose values are useful and important to clients of the class. We consciously decide to break encapsulation here and provide this data to our clients. The size of a collection may serve as a nice example. Fields on the other hand store state or dependencies our class needs to be fully operational. Datastructures like arrays, data access objects (DAOs) or some kind of notification service may serve as examples here.

The internal implementation of both, properties and fields, should never be exposed because this truely breaks encapsulation and takes away the freedom of the class implementor to change their implementation. At a later time you may decide to compute a value or read it from a database instead of storing it directly . On the other hand properties themselves may well be public and belong to the API of our class.

Now on to Java. There is no native property support in the Java language as it does not support the uniform access principle using language constructs. In other languages like Python, Ruby, Groovy or Scala you can change from direct field access to accessor methods without changing the clients, so it is no problem to expose fields (or more precisely properties) and thus make them public or protected. To gain the same degree of freedom in Java you have to emulate properties by using the getter/setter convention of Java Beans. You have to trade conciseness of public fields against this freedom and you really should do it. An IDE can generate the accessors and fold the methods away from your sight. The cost of getters/setters is really negligible.

Now we can derive the conclusions for Java programers. With each member variable you introduce you have to decide if it is a property or just some internal field. For properties you may provide getters and/or setters with appropriate visibility when needed. That means you should not provide accessor methods for all of your fields. In general you should never expose fields directly and all instance variables should be private. Not doing so will remove the freedom to change class internals without affecting the clients. Once a class with exposed internals is published as part of an API it is almost impossible to change internal design decisions.