Skip to Content

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.png

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.PNG

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.PNG

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.PNG

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 Conatiners 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.png

To report this post you need to login first.

3 Comments

You must be Logged on to comment or reply to a post.

  1. Paul Hardy

    Greetings,

    This is very good, but even after six years of playing around with object orientated programming, and my recent flurry of reading every internet article I can find on the subject e.g. by Robert C.Martin, Martin Fowler etc, I am still struggling. So I have a question or two, about the best way to unit test enable something.

    I have been using constrctor injection when unit testing, I create a subclass of the real repository / database accesss / persitency layer class, and pass it into an optional parameter when calling the “get instance” static method of the class under test. Above you say the database object class should have an interface and then both the real class and any mock class implement that interface. I am still not clear at all on why that is better than the mock being a subclass of the original. I am sure it IS better, but can you have another go at explaing why?

    Also, you mention you could mock up an object using a mocking framework, or by doing it manually. I have thus far been doing this manually i.e. creating an internal table of LIPS or whatever I am testing and filling in the pertinent values needed for the unit test.

    There is not a mocking framework built into ABAP as yet, is this correct? Unless there is one on the code exchange?

    Even if there was, I would still have to add the values I want somehow? E.g. if my test is that the block of code under test should reject purchase order items flagged for deletion, my mock purchase order item should still somehow know one of items was deleted?

    P.S. I am working on my blog about the best way to incrementally refactor procedural code, I have a monster application in my company, which our whole business hangs off, but it is 100% procedural and over it’s 15 years of evolution it has been incremently added to by dozens of programmers and it is literally impossible to understand or even make the simplest modification without hours of study, even by someone like me who has been there the whole time, let alone someone new.

    This is what we are aiming for, to change such an appliaction such that a new person CAN modify it easily without bringing the whole business crashing down around our ears!

    Mere words cannot say how happy I am that there are now lots of blogs about OO design in ABAP floating around the SDN.

    Cheersy Cheers

    Paul

    (0) 
    1. Paul Hardy

      P.S. I liked your blogs so much I decided to “follow” you on the SDN, and I presumed I would then get an email each time you posted a new blog or something.

      This is clearly not the case. Is “following” someone on SDN sort of just a tick of approval?

      (0) 
    2. Uwe Kunath Post author

      Dear Paul,

      thank you very much. Of course it was also a feasible way to mock an existing object by subclassing it. The only issues which might occur is, that subclassing final classes is not possible and you would have to take care of any parent classe’s constructor parameters and implementation.

      Also, while in the development process I usually cannot guarantee, that subclass and base class stay in a strong functional relationship at all the time: As interfaces usually allow you to replace the class completely later on, base classes only allow to use one of their subclasses. This way, you are often forced to violate the Liskov Substitution Principle, in case replacing the class e.g. in refactorings is needed. But from the functional point of view it would also work.

      In fact I am currently dealing with this topic as we have written our own Mocking Framework and IoC Container for internal development purposes.  Both will have to provide subclass-support in later releases.

      Links can be found in this article.

      A mock object can be easily defined by returning conditional results based on the caller’s inputs, for example

      DATA: ls_lips1 TYPE lips.

      DATA: ls_lips2 TYPE lips.

      ls_lips1 = …

      ls_lips2 = …

      lo_mocker->/leos/if_mocker=>mock( ‘ZCL_LIPS_ACCESS’ )->method( ‘get_item’ )->with( 10 )->returns( ls_lips1 ).

      lo_mocker->/leos/if_mocker=>mock( ‘ZCL_LIPS_ACCESS’ )->method( ‘get_item’ )->with( 20 )->returns( ls_lips2 ).

      Delete operations may be simulated by taking into account also the numbers of previous calls to this method (this feature is not yet implemented):

      DATA: ls_lips1 TYPE lips.

      ls_lips1 = …

      lo_mocker->/leos/if_mocker=>mock( ‘ZCL_LIPS_ACCESS’ )->method( ‘get_item’ )->with( 10 )->returns( ls_lips1 ).

      clear: ls_lips1.

      lo_mocker->/leos/if_mocker=>mock( ‘ZCL_LIPS_ACCESS’ )->method( ‘delete_item’ )->with( 10 )->returns( abap_true ).

      lo_mocker->/leos/if_mocker=>mock( ‘ZCL_LIPS_ACCESS’ )->method( ‘get_item’ )->with( 10 )->returns_at_second_time( ls_lips1 ).

      You may consider to also throw excetions if needed:

      lo_mocker->/leos/if_mocker=>mock( ‘ZCL_LIPS_ACCESS’ )->method( ‘delete_item’ )->with( 20 )->raises( ‘ZCX_LIPS_ACCESS’ ).

      As Code Exchange does not allow for a company like us to upload the project , it is not an option for us. Furthermore, the project is in our own namespace which is not compatible with Codex.

      So any delivery would have to be discussed individually with the one who is interested in the framework.

      Regards,

      Uwe

      PS: I’m also follwing you and obviously I also get no mails. No idea what this feature is all about then…

      (0) 

Leave a Reply