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:
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.
And this is the output by the program.
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.
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)
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.
Character Parsing
Every logical chunk representing a real character which is relevant for the output, must be parsed.
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.
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:
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.
Great blog, Uwe, and an excellent example, too! I warn you that I might "steal" it on another occasion (concerning parser generators)* 🙂
I found that light-weight classes often generate a bit too much syntactical noise in ABAP. In Java or C++, light-weight classes are favoured by the language itself. In ABAP, they are possible, but with unclear benefit in many cases.
For example:
It is a common refactoring pattern to "replace type code by class". The idea is to replace a "case" or "if/else/else/else..." construct (which is considered bad) by the call of an appropriate subclass, like so (I intentionally write the code in upper case, to give it this nice COBOL-80es-feeling that we all like so much 🙂 ):
Old code:
CASE IV_TAXTYPE.
WHEN GC_TAXTYPE-VAT.
EV_RESULT = IV_VALUE*(1+GC_TAX_RATE-VAT).
WHEN GC_TAXTYPE-INCOME.
...
WHEN GC_TAXTYPE-ENVIRONMENTAL.
...
WHEN GC_TAXTYPE-PROPERTY.
...
WHEN OTHERS.
RAISE EXCEPTION TYPE ZCX_ILLEGAL_TAX_TYPE
EXPORTING
TAX_TYPE = IV_TAXTYPE.
ENDCASE.
New code:
EV_RESULT = ZCL_TAX=>TYPE( IV_TAXTYPE )->COMPUTE( IV_VALUE ).
After the refactorization, each tax calculation algorithm lives in its own leight-weight class, each of which implements a "compute" interface, and the instances are managed or provided in the ZCL_TAX class.
One can do this in ABAP - however, the readability will usually not improve. Leaving out the code for the creation of the instances, we will need the definition of the interface:
INTERFACE ZIF_TAX_ALGORITHM DEFINITION.
PUBLIC SECTION.
METHODS COMPUTE IMPORTING IV_VALUE TYPE NUMERIC
RETURNING VALUE(EV_RESULT) TYPE DECFLOAT34.
ENDINTERFACE.
Then, we will have to define for each tax type the corresponding implementing class:
CLASS LCL_TAX_ALGORITHM_VAT DEFINITION.
PUBLIC SECTION.
INTERFACES ZIF_TAX_ALGORITHM.
ENDCLASS.
And the specific algorithm now sits in this little box:
CLASS LCL_TAX_ALGORITHM_VAT IMPLEMENTATION.
METHOD ZIF_TAX_ALGORITHM~COMPUTE.
EV_RESULT = IV_VALUE*(1+GC_TAX_RATE-VAT).
ENDMETHOD.
ENDCLASS.
These code blocks - class definition and class implementation - have to be repeated for each tax type - the only difference will be the line which computes the specific tax.
Even when leaving out the instance creation part, this is commonly considered as "too much ado". And the readability is indeed worse than the original.
So here, the limits of the language (ABAP) are also limits to refactoring. In order to apply this refactoring pattern, the language should allow it to exhibit the essential part - the statement in which the tax is really applied - and reduce the syntactical noise.
Cheers,
Rüdiger
*giving the reference, of course!
Dear Rüdiger, thank your very much for your feedback. In fact, if the tax calculation stayed as simple as you mentioned (1 line of code), the CASE-approach would be more readable.
In any other case I would stick to the refactored solution as more complex tax calculations within CASE-statements are IMHO harder to read.
By the way: IoC Container can help you in resolving the correct class per tax type. Basically, resolving single instances on a request basis (usage as service locator pattern) is not their main purpose, but the functionality which would be required to do this is included out-of-the-box
Dear Uwe, thank you for your reply. I have chosen intentionally one-line implementations, since we are talking on the problems of light-weight classes in ABAP. We don't talk about heavy-weight classes - having the "usual size" of, say, a non-OO modularization unit like a function group or a subroutine pool.
Even with dependency injection / IoC, the syntactical noise remains a problem for those small classes. They have to be defined and implemented with the above skeleton anyway. Compare it with the ease with which you can implement an interface in a language like Java.
This is not to demean ABAP - I am fond of ABAP! - but only to show a limitation coming from the structure of the language.
Cheers,
Rüdiger
I'm reading "Clean Code" by Robert Martin as well.
There is so much there that can be re-used in the ABAP world...
Awesome blog Uwe. Waiting for more such problems solution.
Nice read!
Found it via the "Related Blog Posts" on https://blogs.sap.com/2019/05/03/clean-abap/ 😉
best
Joachim