Getting the Brownfield clean, but not green - Part I

[former_member182680]

Initial situation

Imagine a legacy application which has grown over the past few years. It has evolved for generations, maintained by several generations of developers. While the first generation of developers had a clear vision of what the application should do and what the design should be, this vision was lost somewhere in between. The application cannot be understood easily and specific documentation would be needed to understand implementation details. As developers often run out of time, this documentation is not existent at all.

As a new developer in an existing application, you often face this situation. If the application’s structure is not clear you will have a hard time to figure out what the core concepts are and how to make enhancements to the existing code.

A historically grown application which tracks movements of objects through storage bins could consist of the following three entities:

• Movable object
• Storage bin
• Movement activity

The main task of this example application is to keep track of the current storage bin of the moveable objects and their movement activities in the past. So here is the simplified class diagram:

This class diagram looks quite handy at the beginning. In the next chapters I’m going to show what went wrong and what makes future enhancement unnecessarily complicated.

Tell, don’t ask

ZCL_MOVEMENT_ACTIVITY expects a movable object as well as a destination to work properly. This data is passed as an ID of the movable object via the constructor (which eventually leads to a flat structure Z_MOVEABLE_OBJECT) and the method +SET_DESTINATION(IS_DESTINATION : Z_STORAGE_BIN) . The intention is probably to provide some data sources in order to allow ZCL_MOVEMENT_ACTIVITY make some assumptions about the data it got.

This is not a good practice.

Example: If there was an object status which indicates of the object is blocked for any movements, the activity class needs to read the status from the structure Z_MOVEABLE_OBJECT. Also, it needs to make a decision based on this status, if the object is blocked for movements or not.

If there was another class, which would need to list all blocked moveable objects, it would need to perform this interpretation on the status field again und you would probably need to copy the status interpretation logic into some other method. This also violates the “Don’t repeat yourself” principle.

This way, you make assumptions on foreign objects. Instead of telling, you ask for something and interpret the object’s data somewhere outside of the object.

Don’t repeat yourself

As mentioned before, don’t repeat yourself. This also applies to the logic which needs to evaluate the structure Z_STORAGE_BIN. Again, you are getting inveigled to make some assumptions about another object. Whenever you want to perform the assumption and an interpretation about that again, the fastest way to do it is to copy it, which is a clear smell.

Example: If the storage bin is completely reserved with other movements, is something that needs to be retrieved from the data passed in Z_MOVEABLE_OBJECT, either directly or indirectly.

If there is no central interpretation logic for this, the logic will need to be implemented wherever it is needed – you are starting to repeat yourself.

Single responsibility principle

Take a look at the three classes again. Is there anything they have in common?

You might recognize the SAVE() Method at the end of each methods list.

So what are the classes are intended to do?

• ZCL_MOVEABLE_OBJECT: carry moveable objects data and read it from DB and store it somewhere
• ZCL_MOVEMENT_ACTIVITY: perform movements and store these movements somewhere
• ZCL_STORAGE_BIN: carry storage bin data and store it somewhere

Found the smell?

Everytime you need to use an “and” in your class’ description, there could be smell. In fact, in all of these three classes, there is more than one responsibility for each of them: Besides their core competency, they are obviously also caring about the persistence of their data, in most of the cases this is where some update modules are called or open SQL statements are called directly.

Inappropriate Intimacy

Take a look at ZCL_MOVEMENT_ACTIVITY. What would need to be done if the class was interested in the current storage bin of the moveable object passed to it’s constructor via it’s ID? In case this information was included in the structure Z_MOVEABLE_OBJECT, which can be loaded using the method LOAD_DATA in ZCL_MOVEABLE_OBJECT, we would be fine.

But more often, these data structures directly inherit from database tables, and often, the bin status report would have its own database table.

This means, as a lack of appropriate access functionality in the current class diagram, the developer would certainly read the data directly from the database table. The developer would make assumptions about the implementation of the method +SET_CURRENT_STORAGE_BIN(IS_STORAGE_BIN : Z_STORAGE_BIN)  from outside of the core class ZCL_MOVEABLE_OBJECT. This is a clear sign of inappropriate intimacy. ZCL_MOVEMENT_ACTIVITY would be dependent of implementation details of ZCL_MOVEABLE_OBJECT.

Often this smell regularly leads to another smell, “Don’t repeat yourself”, since this retrieval logic is often just copied wherever needed.

Testability

At some point of time you may decide to unit test ZCL_MOVEMENT_ACTIVITY. You figure out, that it expects an object ID in the constructor which likely leads to a call to ZCL_MOVEABLE_OBJECT=>LOAD_DATA(…) in order to retrieve the data which is needed to get the moveable object.

If ZCL_MOVEABLE_OBJECT=>LOAD_DATA(…) is not called, there needs to be database SELECT for the moveable object which is even worse.

In any case, just to pass a moveable object to the test, you need to prepare the database in the SETUP routine and clean it up in the TEARDOWN routine. This is hard work compared to the benefit you expect from a unit test.

Uli once told me, a good architecture is quite often an architecture which can be easily tested and I think this guy is right.

Conclusion

You have seen three classes and at least five different smells. The list of smells is not a complete list, there could be more, but this would go beyond the scope of this blog post. In real life applications you easily have several hundreds of classes. Every smell decreases reusability of your application’s components, makes it more and more inflexible and in the end very hard to maintain. This is where bugs may arise quite easy.

So what should we do? Throwing the application away is not an option. There will be no green field where we can start from scratch. Our options are refactoring and introducing unit tests which also serve as documentation for the behavior of specific functionalities to some extent. We need to get the brownfield clean, but not green. But this would be the subject of my next blog post.