IoC implemented

Challenge

Yesterday we had an interesting discussion about the following requirement: There is an application which consists of a webdynpro UI and a backend model. The backend model is described by an interface ZIF_SOME_BACKEND_MODEL which is implemented by some specific classes ZCL_BACKEND_MODEL_A and ZCL_BACKEND_MODEL_B. Backend A and Backend B are performing their tasks quite differently, however, the UI does not really care about this fact, as it uses only the interface ZIF_SOME_BACKEND_MODEL.
IoC implemented - Initial situation

Model A and B may be switched depending on some constraints which can be defined by the user but which we are not discussing here.
In case the application uses that backend model A in the backend, everything is okay and the application does what it is expected to do.
But: In case ZCL_BACKEND_MODEL_B does its work, an output stating e.g. that the backend data is derived by an external system should be shown as an info message in webdynpro.

This simple requirement led to two approaches.

Approach #01

The first approach is to apply specific methods which allow the UI to get some kind of a message from the backend.
IoC implemented - Approach 01
This means, method GET_INFO_MESSAGE ( ) needs to be called by the UI and depending on wether or not some message has been specified, the message has to be displayed.
This is a quite simple approach.

Disadvantage of approach #1

But there is also a disadvantage to this approach: By extended the interface with the GET_INFO_MESSAGE-method, we introduce a new concern to the application, which is completely useless for backend model A. Only backend model B will ever provide some messages to its caller (which is, the UI).
Even worse: Every implementer of ZIF_SOME_BACKEND_MODEL will have to implement at least an empty implementation of that method to avoid runtime exceptions when a not implemented method is called.

Approach #2

The second approach makes use of the Inversion of Control principle. Instead of asking the backend something, the UI tells it to tell its message manager what it thinks is newsworthy to tell.
How does it work? The most crucial difference is the existence of a new interface ZIF_I_AM_COMMUNICATIVE. It is implemented only by Backend Model B.
IoC implemented - Approach 02

What happens in the application? The UI tries to downcast the backend model to an instance of type ZIF_I_AM_COMMUNICATIVE. If backend model A is currently used in the application, the downcast will fail and the exception CX_SY_MOVE_CAST_ERROR may be catched silently.
In case the downcast succeeds, the webdynpro message manager of the UI component will be transferred to the backend model.
This gives backend model B the opportunity to show a warning / info message to the UI informing the user that backend model B is active. This could happen at a time point of time which can be controlled by the backend, not by the UI.

Disadvantage of approach #2

But also approach #2 has its disadvantages. By introducing a webdnypro interface to the backend, we have not only a dependency of the UI to the backend, but also bring in a dependency in reverse. At least as far it concerns ZCL_BACKEND_MODEL_B.
Usually you should avoid having such kind of dependencies as they might imply problems when working with dependency tracking techniques such as IoC Containers.
Also, the coupling between backend model B and the webdynpro runtime is increased by introducing the interface IF_WD_MESSAGE_MANAGER to the backend class.
To avoid these issues you might consider to wrap IF_WD_MESSAGE_MANAGER inside another, more abstract class, e.g. a generic message manager interface whose implementers may also work with classic dynpro screens. Instead of the webdynpro message manager, this wrapper class instance would then be injected to the backend. However such an approach might be kind of over-engineering in the first step.

What to do?

We decided to go for approach #2 as it was the least invasive and the most flexible one. We might consider implementing the refactoring mentioned in the disadvantages section of approach #2 in the future. However, the approach works like a charm right now.

Advertisements

The issue with having many small classes

Aftermath

The previous blog post was about applying design principles to the architecture of a simple parser application, which served as an exercise application. It led to quite a few, small classes in the end which all had their own responsibility.
Big Picture

Issue

As most of the classes deal with solving a certain aspect of the exercise, one class has the responsibility to arrange the work of the other classes to solve what needs to be solved. This class implements the interface ZIF_OCR_PARSING_ALGORITHM . An instance of that class is given to every consumer which needs to parse some sample input.
Every low level class that this class is dealing with to “orchestrate” the program flow, is registered as an dependency. Dependencies can be declared in the constructor of this course grained class. This makes creation of that class impossible without satisfying its dependencies.

methods CONSTRUCTOR
importing
!IO_FILE_ACCESS type ref to ZIF_OCR_FILE_ACCESS
!IO_LINES_SEPERATOR type ref to ZIF_OCR_LINES_SEPERATOR
!IO_CHARACTERS_SEPERATOR type ref to ZIF_OCR_CHARACTERS_SEPERATOR
!IO_CHARACTER_PARSER type ref to ZIF_OCR_CHARACTER_PARSER .

Providing dependencies during object creation by an external caller is called “dependency injection” (DI) as opposed to having classes that create their dependencies on their own. As the creation of the low level instances is not in the control of the class that orchestrates the parser flow, this paradigm is also called “Inversion of control” (IoC).
However, this approach has another challenge ready. That issue has been described very clearly by Oliver as a reply to the previous blog post
“Injection in the constructor – even to an interface – looks like quite tight coupling to me as it leaves the consumer of the coarse-grained class (the algorithm, in your case) to instantiate the composites.”

Factory

Of course the consumer of the composite object model should not create it on its own. One possibility would be to utilize the factory pattern. Instead of composing the main object and all of its dependent objects on its own, the consumer delegates this task to a factory method, which could be a static method call in its most simple realization:
DATA lo_parser TYPE REF TO ZIF_OCR_PARSING_ALGORITHM.
lo_parser = ZCL_PARSER_FACTORY=>CREATE_NEW_PARSER( ).

Within CREATE_NEW_PARSER( ) the same magic takes place as before: in the first step, all low level objects are created, in the second step the main parser object is created, while its dependencies declared by the CONSTRUCTOR are satisfied. This approach is called “Poor man’s dependency injection”

*alternative 1: poor man's DI
DATA lo_ocr TYPE REF TO zif_ocr_parsing_algorithm.
DATA lo_char_sep TYPE REF TO zif_ocr_characters_seperator.
DATA lo_char_parser TYPE REF TO zif_ocr_character_parser.
DATA lo_file_access TYPE REF TO zif_ocr_file_access.
DATA lo_lines_seperator TYPE REF TO zif_ocr_lines_seperator.
CREATE OBJECT lo_char_sep TYPE zcl_ocr_characters_seperator.
CREATE OBJECT lo_char_parser TYPE zcl_ocr_character_parser.
CREATE OBJECT lo_file_access TYPE zcl_ocr_gui_file_access.
CREATE OBJECT lo_lines_seperator TYPE zcl_ocr_lines_seperator.

CREATE OBJECT lo_ocr TYPE zcl_ocr_parsing_algorithm
EXPORTING
io_character_parser = lo_char_parser
io_characters_seperator = lo_char_sep
io_file_access = lo_file_access
io_lines_seperator = lo_lines_seperator.

IoC Container

However, the task of manually creating dependent objects in order to hand them over to a course grained object when it is created, can be automated. Think of the facts that are known:

  • all low level interfaces like ZIF_OCR_CHARACTER_PARSER have one class that implements each of them
  • instances of these classes could be created dynamically, as their constructors currently require no input parameter
  • the course grained interface ZIF_OCR_PARSING_ALGORITHM also has a specific class that implements it
  • however, its constructor requires some instances of the low level interfaces
  • as these required parameters are instances of the low level interfaces, these dependencies could be satisfied automatically

As a brief summary, this is exactly what an IoC Container should do, also refered to as DI containers.
The registration of the interfaces and classes is done programmatically in the following example. However, it may be moved to customizing in order to “free” the code of having hard references to explizit class names

*alternative 2: IoC Container
DATA lo_ioc_container TYPE REF TO /leos/if_ioc_container.
lo_ioc_container = /leos/cl_ioc_container=>create_empty_instance( ).
lo_ioc_container->add_implementer( iv_contract = 'zif_ocr_characters_seperator' iv_implementer = 'zcl_ocr_characters_seperator' ).
lo_ioc_container->add_implementer( iv_contract = 'zif_ocr_character_parser' iv_implementer = 'zcl_ocr_character_parser' ).
lo_ioc_container->add_implementer( iv_contract = 'zif_ocr_file_access' iv_implementer = 'zcl_ocr_gui_file_access' ).
lo_ioc_container->add_implementer( iv_contract = 'zif_ocr_lines_seperator' iv_implementer = 'zcl_ocr_lines_seperator' ).
lo_ioc_container->add_implementer( iv_contract = 'zif_ocr_parsing_algorithm' iv_implementer = 'zcl_ocr_parsing_algorithm' ).

No matter, how the classes have been registered, either in the code or by customizing, the caller now only needs to call the container and request an instance from it at startup:

DATA lo_ocr TYPE REF TO zif_ocr_parsing_algorithm.
*create the container either by reference to customizing or by the registration coding shown above...
*...
lo_ocr ?= lo_ioc_container->get_implementer( iv_contract = 'zif_ocr_parsing_algorithm' ).

Why should we write clean code?

Dojo

Recently I took part in an architecture dojo. This is an exercise, where a certain simplified example of a program specification is given. The participants are asked to propose an architecture how the task can be solved. Architecture dojos usually do not focus on the code level, rather on a more course grained level dealing with the structure of the program rather than the code itself.
The goal is to present a good architecture in the end after a certain period of time. What makes this architecture good and what makes it bad? The outcome should meet certain requirements like evolvability, testability and the alignment to clean code principles.

The specification

There is OCR software which scans paper documents with document numbers. It produces a file with multiple entries.
One logical line consists of three physical lines and may look like this:
Digits
Each character consists of three horizontal characters over four lines. Only pipes and underscores are allowed and can be arranged to represent a certain number.
The fourth line contains no data, as it serves as delimiter for the next document number.
You will have to write a program that reads this file from a client computer, parses it and outputs the actual document numbers on the screen.
Please keep in mind: In the future, the source where the files are uploaded from may change from client side storage to server side storage. Additionally the solution should be easily testable using unit tests.

Executing the program

Of course the task of this blog post was not to implement a program. But here it is.
This is the sample file content.
Sample File
And this is the output by the program.
sample output

Bringing design principles into play

To achieve the requirements given in the specification, a single monolithic program wouldn’t be an option.
A single program which reads the file, converts and parses it within one huge method, wouldn’t be easily testable.
Additionally, any other source where the file may come from would have to be coded directly into that method. This change would cause a break to the encapsulation.
This brings us to the question how clean code principles might help us to enforce a good architecture.
Especially the following principles should be considered:

  • Single Responsibility Principle (SRP)
  • Open Closed Principle (OCP)
  • Single Level of Abstraction (SLA)

No more buzzwords!

What do all these principles mean? Let’s have a short briefing.

Single Responsibility Principle (SRP)

A class should have only one responsibility. Always try to give the class, that you are currently developing, a description of what it does. If you need to use the word “and”, or you can only hardly cope without using it, there could be a violation to SRP.

Open Closed Principle (OCP)

A class should be open for extensions, but closed for modifications. This usually only works for some kinds of changes, but not for every thinkable future change. A list of what could change with the program, which we need to design in the dojo, is mentioned within the specification.
For example, if the way how the file is going to be accessed changes, the change should not affect existing code in the core program. Every new data source will have to be made available through extension, but not through modification.
But please remember: As often, the list of the above mentioned possible changes might not be a complete one.

Single Level of Abstraction (SLA)

“Each piece of code should talk on a distinctive level of granularity. Don’t mix implementation details with invocation of high-level abstractions. Refactor code to balance the level of abstraction.” (http://lumiera.org/project/background/CleanCodeDevelopment.html)
If we apply this principle to the future design, there should be at least distinct methods to access the file contents, to group the logical lines into sets of physical lines, or to split the logical characters which cover multiple physical lines into distinct 3 character X 4 lines chunks. The calls to these and certain more methods will have to be arranged by another and more course-grained method.

Okay, so what needs to be done?

Let’s get an overview of the different responsibilities of the program. First of all, there will be some kind of file access. When we got the file into the memory, it should be separated into chunks of four lines each. This is another responsibility which actually has nothing to do with how the file is accessed. Finally, each chunk consisting of string lines will need to be separated by another functionality, which extracts the characters out of the chunks. As these characters are still somehow encodes (as every character spans 3 characters x 4 lines in the file contents) there will be another function that parses these characters. In the end, the parsed characters will need to be concatenated and returned by a component which arranges the calls to the described functionalities.

File access

The access to the file contents is a functionality which will be replaced later on by another implementation (client side upload vs. server side upload). This is what is already mentioned in the specification. We decide to implement that each of these functionalities is implemented by a specific class. These classes will need to implement a common interface as callers to these functionalities should not really need to care about how the file access is implemented in particular. The first implementation of this interface will have to read a file from the client computer.
ZIF_OCR_FILE_ACCESS

Lines Separation

Each logical line which represents one account number consists of four physical lines. We will need to split these into chunks of four lines each. This responsibility is specified by another interface.
If any detail relevant for the line separation should change in the future (e.g. if another empty line besides the fourth one is introduced), the caller of this functionality should not really care about what is going on. Instead, we just describe the functionality by an interface which will also need to be implemented by a specific class later on. The output will be TYPE TABLE OF STRING_TABLE (custom type)
ZIF_OCR_LINES_SEPERATOR

Character separation

Each line which consists of four physical lines should be separated into chunks which represent the logical characters (that is, chunks of 3 x 4 characters). So here is another interface which describes this interaction. Character separation will take place for each logical line, so the input will be STRING_TABLE and the output will be another custom type which represents a nested table with logical characters. A logical character representation is TYPE TABLE OF CHAR03, a collection of them is just another nested table of TYPE ZOCR_T_LOGICAL_CHARACTERS.
ZIF_OCR_CHARACTERS_SEPERATOR

Character Parsing

Every logical chunk representing a real character which is relevant for the output, must be parsed.
ZIF_OCR_CHARACTER_PARSER

Who is the organizer?

Of course there should be a class which organizes the calls to the string manipulation and file access components.
Here it is. The interface requires the method to return a list of account numbers in string format.
ZIF_OCR_PARSING_ALGORITHM

The whole picture

What does it look like now? How does the implementer of ZIF_OCR_PARSING_ALGORITHM know all the other components which are to be used?
The answer is “constructor injection”. Every dependency is handed over to the implementer of ZIF_OCR_PARSING_ALGORITHM when the CREATE OBJECT statement to create this instance is executed. These dependencies are instances of specific classes on their own. They implement the already introduced interfaces.
There will be another caller which puts all of these functional bricks together, but for now, this information should be comprehensive enough to form a big picture:
Big Picture

Benefits

This approach of having such small classes might look a little bit strange for someone who developed in ABAP for years. But it has its benefits. The resulting structure is easily maintainable as interfaces hide the implementation details of classes and their sub-components (that is, more fine-grained, other classes). The implementations behind these interfaces can be switched easily without changing existing code within the core parser code.
Additionally, unit tests can be applied quite easy to each of these implementations. Unit tests have the goal to document and test behavior consistently, comprehensible and repeatable. This safes testing efforts when in integration tests and tracks down possible issues with the code to specific classes just by executing the unit within milliseconds.
Writing a testable program and applying unit tests to it usually takes up two to three times of what it would take to write it quick & dirty. But when it comes to testing and when issues start to arise, the test-based approach takes up speed in terms of defect rate and time to localize an error. In the end, it usually safes more time than it costs you to do it properly.

Getting the Brownfield clean, but not green – Part II

Initial situation

In my previous blog post, I introduced an architecture which could be the backbone of a historically grown application. Certain design issues, called smells, were introduced which could prevent a new developer in the project from its main task, which is adding certain extra functionality to the existing application. Certain smells, that contradict an evolvable architecture, need to be eliminated. This process of improving the existing code before applying new functionality is called Refactoring, which is the subject of today’s blog post.
A good definition for an entity in this blog can be found in Domain Driven Design by Eric Evans. In short, an entity can be identified by its key and is an independent object that usually has a real life representation, as this is the case for movable objects or storage bins and even activities as they are a business document.
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:

Sample Legacy Application

Tell, don’t ask

ZCL_MOVEMENT_ACTIVITY expects a movable object as well as a destination to work properly. This data is passed as a flat structure via the constructor 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.

Providing structures as method input is a common anti-pattern in ABAP development. The habit certainly comes from programming with function modules, where your only opportunity to hand over complex data structures was to provide flat or nested structures or even internal tables.
In ABAP OO, you don’t need to do it. Think of structures differently: Objects are better structures since the can include basically the same data that can be included also by structures, with the difference of adding behavior to them. This behavior could also be restricted, which means that certain fields in the structure may not be accessible. This behavior could be validation which is processed before a certain field in the structure is populated.
The appropriate class in the example would be ZCL_STORAGE_BIN. As you can see, ZCL_STORAGE_BIN defines the attribute MS_STORAGE_BIN. In addition you can later add certain instance methods like IS_BLOCKED( ) or CHECK_DEST4MOVEABLE_OBJ(IO_MOVEABLE_OBJCT: ZCL_MOVEABLE_OBJECT) in order to support quantity checks or checks, if the storage bin may serve as a destination for an object’s movement. The method’s signature wouldn’t even need to change.
So instead of interpreting the data you got in your method, you are now telling the passed object to return some data to you which is needed for processing. Since the only data which is accessible for you can be accessed by the object’s methods (besides public attributes, never ever do that!), data encapsulation is far better in comparison to use simple structures, which cannot transfer any logic at all. A well written class is a pleasure for the developers who need to work with it. However, a badly written class may still expose to much information to it’s callers, but this is another smell.
Solution: The best approach would be to completely replace the signature of +SET_DESTINATION(IS_DESTINATION : Z_STORAGE_BIN)
With
+SET_DESTINATION(IO_DESTINATION: ZCL_STORAGE_BIN)
Inside the method, call appropriate Get-Methods of the passed object, or the common Is-Methods like IS_BLOCKED( ).
If this is not possible, introduce another, new method which is subject to future developments. The old method is marked as OBSOLETE in the comments or method’s description or both. Please do not forget to also include a hint of where the new method is located since one of the most annoying issues for a developer is to use an obviously deprecated method, which lacks the link to its newer version.
If the structure Z_STORAGE_BIN contains the storage bin key, you could consider another solution. The old method +SET_DESTINATION(IS_DESTINATION : Z_STORAGE_BIN) will remain as it is. But internally, you call a factory or repository to retrieve the storage bin’s object representation by its key attributes.

Storage Bin Repository

The class diagram currently gives you no hint about instance creation control patterns. You could consider to implement the repository as a Singleton, but this would usually prevent IoC Containers to create the repository for you as the contructor is usually made protected in these patterns. IoC Containers may bring you huge advantages when it comes to unit tests but this topic deserves its own section.

Don’t repeat yourself

Any logic hat has a natural relevance to the entity where it is related to should need to be implemented closely to the specific class which implements this entity.
This also applies to a status check on the movable object, wether it is blocked for movement activities or not. Don’t try to make this decision outside of the class ZCL_MOVEABLE_OBJECT, based on a STATUS-field in Z_MOVEABLE_OBJECT which may be retrieved by IV_MOVEABLE OBJECT in the constructor of ZCL_MOVEMENT_ACTIVITY.
Even if the status attribute only contains two possible values, “B” for “Blocked” and “R” for “Ready”, don’t do that. Playing with fire usually leads to burned fingers.
Instead, try to get an instance of type ZCL_MOVEABLE_OBJECT as fast as possible. This could happen by a method parameter or, again, a repository lookup. Implement the interpretation upon the status where it belongs to (ZCL_MOVEABLE_OBJECT).
To come back to the topic of this section: Interpreting the object’s status outside of the object’s main class usually leads to other code fragments where this interpretation is done again as the main class lacks appropriate methods. This way, 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?

Entity objects should not take care about how they are stored somewhere, for example in the database.
This is not because we like to switch from one database to another in the near future (despite the fact that ABAP offers you very good Open SQL support).
Instead, coupling entities like ZCL_MOVEABLE_OBJECT to the way they are stored usually leads to another issue: Caching requirements will likely be also implemented in ZCL_MOVEABLE_OBJECT.
This is where ZCL_MOVEABLE_OBJECT starts to grow and to serve more than just one purpose.
It is better to leave this task to repositories, as mentioned before. Repositories control the way, how entity objects are created or stored to the database. Hence, our SAVE-implementations should move to their appropriate repositories.

Storage Bin Repository with SAVE-method

Testability

Repositories manage the access to entities. Usually, they are seldom referenced by entities, since the classes that implement these entities should expect other entity instances wherever possible. Sometimes you cannot get an instance directly, for example if the method’s signature needs to stay the same and expects only the ID of the referenced entity from its callers.
Take a look at +SET_DESTINATION(IS_DESTINATION : Z_STORAGE_BIN) of ZCL_MOVEMENT_ACTIVITY again. In case, the key of the storage bin is already included in the structure Z_STORAGE_BIN and you have a repository for this entity, try to ask the repository for an instance inside the method.
However, the repository needs to be registered in the activity instance before. This could happen either via constructor injection or via a RESOLVE_DEPENDENCIES( ) – method which can be called in the constructor.
This method may acquire the instance of the storage bin repository by either calling the constructor, a factory or a static method of ZCL_STORAGE_BIN_REPOSITORY which implements the singleton pattern. If you share only one instance of ZCL_STORAGE_BIN_REPOSITORY in your application you may also utilize caching of objects on instance level.
In order to unit test ZCL_MOVEMENT_ACTIVITY you will need to have sample storage bins as well as sample movable objects. Creating these storage bins and moveable objects will likely cause you to setup database table entries for each test which is quite a big effort.
Instead, it is usually a far better idea, to describe repositories by specific interfaces.
Storage Bin Repository with interface

This way you can access the repository by its interface, not caring about any implementation details in your client’s classes. The implementation behind this interface can be easily mocked either by using a mocking framework or manually.
On the other side, IoC Containers, may serve as service locators in a RESOLVE_DEPENDENCIES( )-method and replace the productive implementation of a repository by a mocked repository in a unit test. The mocked repository will eventually always returns the same instance of a storage bin, no matter what is really stored in the database.
Warning: using IoC Containers as pure service locators is quite dangerous as you would start to call the container all over the place. Instead it is better idea to declare every dependency via dependency injection (CONSTRUCTOR- or SETTER-based) in the entity. This dependency will be satisfied by the entity’s repository, which in turn, declares its own dependencies, for example other repositories. The goal of IoC Containers is to create the backend objects like repositories and satisfy their dependencies automatically for you. In a clean design there is no need to create entities with the help of IoC Containers, because there are already repositories for this task, but this would certainly be a topic for a future blog post.

Result

In the end, if all client calls can be adjusted, the resulting UML diagram could look like this. Of course such a result would be the optimal and most optimistic scenario, but in most of the cases you can come close to this result by utilizing RESOLVE_DEPENDENCY( ) methods as described in the previous sections, without changing the old API at all. This solution wouldn’t be clean, but much better than the current design and certainly more robust.
The goal is not, to change the design of a working application without specific purpose. But in case enhancements would have to be done, it is often better to improve the design where you see obstacles in implementing the desired functionality in the first step.
Afterwards, the enhancement is implemented even faster and more robust, with less testing effort and side effects. New developers understand more easily the task of each class, at the cost of a higher learning curve at the beginning in order to get the whole picture of entity classes, repositories and cross-cutting concerns like IoC Containers or mocking frameworks.
Eventually, unit testing of certain components becomes easier. These unit tests contribute to both software quality as well as even a bit of documentation as you are testing an object in unit tests the way it is intended to be used (respectively not used).
Sample Legacy Application after Refactoring

Getting the Brownfield clean, but not green – Part I

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:
Sample Legacy Application
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.