Code quality

Good quality code should satisfy certain characteristics (they are in the picture).

Code quality: why is it important?

Take a scenario where you need to fix an issue or modify existing code. First, you need to understand code, then modify or write additional code.

This chart shows time spent on related activities.

Code quality and Software cost

If you minimize the time needed to understand the code, it will bring a reduction in the overall time and effort.

Poor code quality and clarity, on opposite side, may lead to:

— functional defects,

— more cost and time to make changes,

— poor application performance,

— extension is problematic,

— increase in technical debt.

Functional code quality – the level of adhering to or meeting the functional requirements. It is about “the way the code is working”. Activities like unit testing and functional testing will ensure functional code quality of the project.

Structural code quality – the quality of the code that was written. So, structural code quality is about “the way code was written.”

Later in this course we will discuss Structural code quality and provide corresponding recommendations.

So, when you take part in a real project then follow the development guidelines used on the project.

Mind the performance

If you implement high performance components, use the advice from this guide with care. Some of approaches described below may make performance slower (more method calls) or demand more memory (more objects). ABAP has some specific features that may increase this (for example it compares data types during a method call). So, splitting a large method into a set of sub-methods may make the code slower.

However, it is strongly recommended to not avoid the code optimization based on obscure fears. Most rules (e.g., naming, commenting) have no negative impact at all. Try to implement things in a clean, object-oriented way. If you suppose that something runs too slow then make a performance measurement. Only after that should you take a fact-based decision to refuse specific rules.

Prefer object orientation instead of imperative programming

Object-oriented programs (classes, interfaces) provide better segmentation and can be changed\refactored\tested less effort than imperative code like programs, functions, etc. But in some situations, you must use imperative objects (a function for a Remote Function Call, a program for a transaction, etc.). In such cases try to use imperative objects as wrappers only and stick to object-oriented style inside them.

Prefer functional instead of procedural constructs

They are usually shorter and understood more natural to modern programmers.

Use design patterns wisely

Implementation of design patterns sometimes demands essential effort. So, use them when they are appropriate and provide essential benefits. Do not do it everywhere just for the sake of it.

Good

Bad

Use names that inform about the content and meaning of variables\objects.

Do not concentrate on the data type or technical information. Usually, they do not facilitate understanding of the code.

Search for proper names in the solution domain (for example, «queue» or «tree») and in the business domain (for example, «invoice» or «posting»).

Do not try to invent your own language. You need to exchange information with your teammates, product owners, customers, so choose names they can understand without a special dictionary.

Use pronounceable names, avoid abbreviations

For example, use detection_object_types instead of something cryptic like dobjt.

If you use some abbreviations, then use the same abbreviations everywhere for the corresponding value.

Class naming

Methods naming

Boolean methods

Function module

For names of classes use nouns and for names of methods use verbs

Start Boolean methods names with verbs like “is_” or “has_”.

It is better to name Function modules like methods.

Good

Bad

Use the same term for a concept; don’t mix it with other synonyms. Synonyms may force the reader to waste time on finding a difference that’s not there.

Use pattern names only if really implement them

Usage of software design patterns names for classes and interfaces is acceptable only if you really implement them (for example, you may name your class form_factory if it really implements the factory design pattern). The most used patterns include singleton, factory, decorator, facade, observer, iterator, etc.

Good

Bad

Use constants instead of magic numbers

Inline declaration

Up-front declaration

Prefer inline in comparison with up-front declarations but avoid inline declaration in optional branches

is preferable then

Do not chain up-front declarations

Chaining suggests that variables are related on a logical level.

Also, chaining makes code reformatting and refactoring a little bit more difficult (changing of variables list requires meddling with colons, dots, and commas, etc.).

Use the right table type

• It is preferable to use HASHED tables for large tables that are filled in a single step, never modified, and read often by their key. Their processing overhead of hash tables only is essential. So, they are suitable for enormous amounts of data with lots of read accesses. Each table’s content change requires expensive recalculation of the hash. It is a bad idea to use hash for tables that are modified too often.
• SORTED table type is good for large tables that need to be sorted at all times, that are filled bit by bit or need to be modified, and read often by one or more full or partial keys or processed in a certain order. Changing content requires finding the right insertion spot, but there is no necessity to adjust the full table’s index. Essential value of Sorted tables may be gained only for large numbers of read accesses.
• For small tables or “arrays” (where indexing produces more overhead than benefit) use STANDARD table type. There you do not care at all for the order of the rows, or you want to process them in exactly the order that rows were appended.

Prefer INSERT INTO TABLE instead of APPEND TO

INSERT INTO TABLE can be used with all table and key types. So, the table’s type and key definitions can be refactored easier if you need to change it due to performance requirements.

APPEND TO is good only for a STANDARD table if you use it in an array-like fashion (entry is added to the table as the last row).

It is better to use

or

Avoid DEFAULT KEY

Often the only aim of adding Default keys is to get some new functional statements working. Default keys ignore numeric data types. So, some obscure mistakes can happen.

It is better to specify the key components explicitly or use EMPTY KEY if key is not necessary at all.

expresses the intent better than

Prefer LINE_EXISTS instead of READ TABLE to check existence of the row

Use ABAP_BOOL for Booleans

Sometimes developers use the generic type char1 instead of BOOLEAN. Although it is technically compatible it is difficult to understand the fact that we are dealing with a Boolean variable.

In some situations it may be necessary to use a data dictionary element (for DynPro fields, etc.). It is not possible to use abap_bool here because it is defined not in the dictionary but in the type pool abap. So, you may use in such situation you may use boole_d or xfeld (it is possible to create your own data element if a custom description is necessary).

Good

Bad

Also avoid comparisons with INITIAL. The reader must recollect a default value of Boolean variable (abap_false).

Is better then

Also, you may use

Prefer XSDBOOL to set Boolean variables

The name xsdbool is a little bit misleading («xsd» prefix suggests that we deal with stuff like the «XML Schema Definition). But this function is good for our aim, because it produces a char1, which is compatible with boolean type abap_bool best.

Also, instead of xsdbool you may use the COND ternary form. Its syntax is a little longer.

is preferable then

Try to make conditions positive

If you create Boolean variable as negative statement it is difficult to understand what you mean sometimes.

Also try to avoid the creation of empty if\else blocks.

Or

Is preferable then

Decompose complex conditions or create for them methods of their own

Instead of

Good

Bad

When you have multiple alternative conditions – prefer CASE to ELSE IF

CASE allows us to easily create a set of alternatives that exclude each other. The statement has simpler syntax, and its performance is better than a series of IFs.

CASE even prevents some errors that can occur during creation of series of the IF-ELSEIFs.

Bad

Keep conditions nesting depth low

It is difficult to understand Nested IFs quickly, an exponential number of test cases is needed for complete coverage.

Decision trees can usually be simplified by using sub-methods or introducing Boolean helper variables.

Prefer instance to static classes

Static classes lose almost all advantages provided by object-oriented programming. During Unit tests creation it is almost impossible to replace productive dependencies (implemented by/inside static classes) with test doubles.

So, when there is a question whether to create a class\method as static, the answer will usually be: no.

One accepted exception to this rule is plain type utils classes. Their methods are completely stateless and basic (as a result they look like ABAP statements or built-in functions).

Prefer composition over inheritance

It is not recommended to build hierarchies of classes with inheritance. It is better to favor composition.

It is difficult to design clean inheritance because you have to follow rules like the Liskov substitution principle. It is also more difficult to understand the hierarchy (you need to understand guiding principles behind it). Inheritance decreases reuse because methods tend to be implemented to be used only by sub-classes. It also complicates refactoring (to change one method you may need to change the whole hierarchy tree).

Composition supposes that you create small, independent objects, each for one specific purpose. It is possible to combine these objects into more complex ones by simple delegation and facade patterns. Composition forces us to create more classes but has no further disadvantages.

But do not let this advice discourage you from usage of inheritance in the right places.

Do not mix stateful and stateless in the same class

If methods provide the same result no matter when and in what order they are run, this approach is considered as Stateless.

Otherwise, the internal state of objects is changed through their methods (stateful programming), meaning this is full of side effects.

Both approaches are acceptable and have their applications. But if you mix these approaches in the same object it makes the code hard to understand. Moreover, code becomes prone to obscure carry-over errors and synchronicity problems. So, it a bad idea to do that.

Create Global classes by default, use local ones only in exceptional cases

Create global classes as default (classes that are visible in the dictionary).

Use local classes only in exceptional cases (for example for extremely specific data structures or to facilitate writing unit tests).

It is impossible to reuse Local classes, they cannot be used elsewhere. Although Local classes can be easily extracted, some people may fail to even find them.

Mark class as FINAL if it is not designed for inheritance

If your class is not designed for inheritance, then you should prevent accidental inheritance by making it as FINAL.

Mark class members as PRIVATE by default (PROTECTED only if needed)

PROTECTED members are visible inside\can be overridden by sub-classes.

By default, internals of classes should be made available to other development objects (including sub-classes) only on a need-to-know basis. If you make information over-available, you can get subtle errors by unexpected redefinitions and hinder code refactoring.

Is better than

Example of dynamic typing is below:

Use functional instead of procedural calls

But remember about exceptions: dynamic typing forbids functional calls. So, resort to the procedural style.

Good

Bad

Omit RECEIVING

Is better than

Tend to use few IMPORTING parameters, at best less than three

If many parameters really make sense, consider combining them into objects or structures.

Is preferable than

Split methods instead of adding OPTIONAL parameters

Optional parameters force callers to ask a question: which parameters are really required? Are parameter combinations valid (maybe one parameter excludes the other)?

Split method instead of Boolean input parameter

If you use Boolean input parameters, it may be an indicator that the method has two purposes instead of one. It would make sense to split the method.

Is better than

Use exactly one type of parameters (RETURNING, EXPORTING, or CHANGING)

A good method does one thing. As a result, it also should return exactly one thing (the exception is when the output parameters of your method form a logical entity). Otherwise, your method does multiple things, and it is better to split it.

Instead of

Prefer RETURNING over EXPORTING

Syntax is shorter.

Use CHANGING sparingly, where suited

CHANGING should be used for cases when an existing variable that was earlier filled is partially updated.

Use EXPORTING to initially fill a previously empty variable.

Clear or overwrite EXPORTING reference parameters

Reference parameters refer to existing memory areas. They may be filled beforehand. So, clear or overwrite EXPORTING variables to provide reliable data.

Do not clear VALUE parameters

Parameters passed by VALUE are handed over as new. So, corresponding memory areas are empty, and it is useless to clear them again.

Since RETURNING parameters are always passed by VALUE, there is no necessity to clear them.

Do only one thing, do it well

Only one thing should be done in method. It should be done in the best way possible.

It is likely that a method does one thing if:

• it has small number of parameters (including absence of Boolean ones)
• it has only one output parameter
• it is relatively small
• It is problematic to extract other meaningful methods
• It is not possible to meaningfully group method’s statements into logical sections

It better to decompose into

Concentrate on the happy path or error handling (do not combine both)

It better to implement inside the method a happy-path method is built for, or the error-handling-behavior. Combining both is a bad idea.

Small method

Keep methods small

Methods should be small (usually not more than 3 to 5 statements).

Of course, sometimes it does not make sense to reduce a large method further. This is usually okay if the method remains focused on one thing.

Is better then

Fail fast

Validate and fail as early as possible.

It is harder to spot and understand later validations. You may waste resources to get there.

or

or

You may use CHECK or RETURN at method start for validation of input

There is no common opinion on whether you should use RETURN or CHECK statements to exit a method if the input does not meet expectations.

CHECK has shorter syntax but traditional form with IF … ELSE… may be understood by people better.

Optimally, methods should provide a result (a filled return parameter, or an exception). Returning nothing should be avoided.

Do not use CHECK in other positions

Instead of

Use exceptions instead of return codes

Exceptions poses multiple advantages in comparison with return codes:

• Your method signatures stay clean: you can return the result of the method as a RETURNING parameter and at the same time have the possibility to throw exceptions alongside. If you use Return codes, you contaminate your signatures with additional parameters for handling errors.
• The caller may process exceptions later. He can simply write down the happy path of his code. It is possible to put the exception-handling CATCH at the very end of method, or completely outside.
• Error details may be provided through Exception’s attributes.
• The system reminds the caller about handling of exceptions. Return codes can be by chance ignored and nobody may notice it.

Instead of

It is better to use your own super classes for exceptions.

It is recommended to create (abstract) super classes for each exception type of your application (instead of sub-classing the foundation classes directly). It will allow you to CATCH all your exceptions and provide the possibility to add common functionality to exceptions (for example, special text handling, etc.).

Instead of

Example of exception implementation with different error codes

It is better to throw one type of exception

In most cases, throwing multiple types of exceptions is useless. Generally, the caller is not interested and not able to distinguish the error situations. Therefore, usually exceptions are handled in the same way — and if so, it is useless to distinguish them.

The best way to recognize different error situations is to use one exception type but create different error messages (error codes) or sub-classes to enable callers to differ error situations.

Throw CX_STATIC_CHECK for manageable exceptions

If the logic flow supposes an exception to occur (failing user input validation, etc.) and be reasonably handled by the receiver, it is better to throw a checked exception inherited from CX_STATIC_CHECK.

Such exception type must be mentioned in method’s signature and have to be caught or forwarded to prevent syntax errors. It is therefore the consumer will need to take care of handling the error situation.

Throw CX_NO_CHECK usually for situations that cannot be handled

If an error is so critical (failure to read a must-have resource, etc.) that the receiver is unlikely to handle it, use CX_NO_CHECK.

CX_NO_CHECK cannot be declared in method signature. This is acceptable because the consumer is not able to do anything useful about exception anyway (but such exception type can be catch in CATCH section if necessary).

Use CX_DYNAMIC_CHECK for avoidable exceptions

CX_DYNAMIC_CHECK should be used in rare cases. Generally, it is recommended to use other exception types. However, it is possible to use this exception type as a replacement for CX_STATIC_CHECK when the caller has full control over whether an exception can occur.

Force dump for unrecoverable situations

If an error is so critical that you are sure the receiver is not able to manage it (failure to acquire memory, etc.), dump instead of throwing an exception.

Is shorter than

Use RAISE EXCEPTION NEW instead of RAISE EXCEPTION TYPE

Good

Bad

Wrap foreign exceptions.

Be independent from the foreign code (catch foreign exceptions and wrap them in your own exception type).

Instead of

Express yourself in code (not in comments)

Good

Bad

Comments do not excuse you for bad names

Instead of

For code segmentation use methods instead of comments

Instead of

Comments should explain the ‘why’ but not the ‘what’

Instead of

Prefer for comments » instead of *

Or

Instead of

Put comments before the code statements they relate to

Design should be described in the design documents, not in the code

Instead of commenting code – delete it

Instead of

Optimize for reading, not for writing

Most of the time developers read code. Writing code takes up a way smaller part of the day.

So, your code formatting should be optimized for reading but not for writing.

Use the Pretty Printer before code activation

Instead of

Do not place more than one code statement per line

Keep a reasonable line length

Try not to exceed line length of 80-120 characters.

We read text easier if the lines are short. Also, it is more comfortable to debug narrower code or compare two sources next to each other.

Instead of

Condense your code

Instead of

Do not add more than one single blank line to separate things

But leave statements ragged if they have nothing to do with each other:

Align assignments when statements belong to the same object, but not to different ones

Instead of

Close brackets at line end

Instead of

Place single parameter calls on one line

Instead of

Use line-breaks for multiple parameters

Otherwise, it is hard to define where one parameter ends and the next one starts

Instead of

Align parameters

Ragged margins make it harder to see where the parameter name ends, and its value begins

Write testable code

Your code should be written in a way that provides the possibility to test it in an automatic fashion.

If you need to refactor your code for this purpose, do it. And do that first (before adding other features).

If you need to add something to legacy code which is badly structured to be tested, refactor this code at least to the extent that will allow you to test your additions.

Readability rules

It is important to create test code even more readable than corresponding productive code. Test code should be so simple that you will still be able to understand it in a year from now. Follow standards and patterns. It will allow your co-workers to quickly get into the code.

Do not write test reports

It is a bad idea to write a test report that calls some stuff in a specific way and repeat that manually to verify by eye that development objects are still working properly when you are working on it. Instead of it automate your testing with unit tests, that provides an automatic assertion and tells you whether the code is still okay.

Doing it, you will spare yourself the effort of writing the unit tests later and save a lot of time for the manual tests.

Test public methods of classes but not private internals

Classes’ public parts (especially the interfaces they implement) are unlikely to change and rather stable. So, unit tests should validate only the public parts to assure they are robust. Protected and private methods, in comparison, may change sometimes. As a result, you will need to refactor needlessly your unit tests after each change.

Do not obsess about coverage

Code coverage is used not to meet some random KPI (Key Performance Indicators). It helps you to find code you forgot to test.

It is useless to make up tests with dummy asserts just to provide the coverage. By leaving some things untested you make it transparent that these parts cannot be safely refactored. You can have perfect tests without 100% coverage.

Instead of

Names of local test classes should reflect their purpose

Use local classes for tests

By pressing Ctrl+Shift+F10 you may run all unit tests from Local test classes associated with a class. If you hide the tests in global classes, you may confuse people (they cannot see them. As a result, people may forget to run these tests).

Instead of

Mainly test interfaces, not classes

Interfaces are usually more stable than classes

.

Good names

Bad names

Reflect in test method names what was given and expected

Appropriate names describe the given and then of the test while bad names describe the when, provide meaningless facts, or are cryptic.

It would be good to add an explanatory comment when the name is too short to provide enough meaning because ABAP allows only 30 characters in method names. The comment may be placed into ABAP Doc or at the beginning of the test method.

If you have many test methods with long names, it can be an indicator that you should divide your test class into several ones. Differences in the givens may be expressed in the class’s names.

Use pattern given-when-then

Implement your test code following the given-when-then paradigm:

1. initialize objects\variables in a given section («given»),

2. call the actual tested piece of code («when»),

3. validate the outcome («then»).

If the given or then sections are very long so that you cannot visually separate the mentioned sections anymore, it would be good to extract sub-methods.

«When» is exactly one call

The «when» section of your test method should contain only one call to the class under test.

When you call various parts of the class under test it is an indicator that the method has no clear purpose and tests more than one thing. So, it is harder to find the cause when the test fails, the reader will not be sure what the exact feature under the test is, etc.

You may use constants to description of purpose and importance of test data

Good

Bad

Use few, focused assertions

Tend to assert only exactly what the test method is for. Do this with a small number of assertions.

If you use too many assertions, it indicates that the method has no clear purpose. The productive and test code are interrelated in too many places: changing a small piece of productive code will demand rewriting a large number of tests even if they are not really connected to the changed feature. A large variety of assertions confuses the reader, it is difficult to find the most important, distinguishing assertions among them.

Good

Bad

Use the proper assert type

Asserts often do more than their name supposes, for example assert_equals include type comparison and provides precise descriptions if values differ. Usage of the wrong, too-common asserts will force the reader to go into the debugger immediately instead of allowing to make suggestion of what is wrong directly from the error message.

If you check for expected exceptions, then use FAIL

Writing custom asserts allows shorten code and avoid duplication

It may spare you from copy-pasting this repeatedly.

• GitHub — Freedomanda/CleanABAP: Guideline to improve the ABAP code quality

“Clean Code A Handbook Of Agile Software Craftsmanship”, Robert C. Martin Series