sap/styleguides

This repository provides SAP style guides for coding and coding-related topics.

SAP Code Style Guides

Continuous Release  ·  Open Source  ·  Grassroots Project  ·  Optional  ·  Ecosystem

This repository provides SAP's style guides for coding.

Programming languages enable us to say the same thing in different ways. While all of them may be correct, some may be more efficient, easier to understand, and more robust than others.

Our style guides want to show up differences and guide you towards code that has a healthy balance between all of these qualities.

Style Guides

• Clean ABAP
• ABAP Code Reviews

Continuous Release

These guides are updated continuously, meaning any change is reviewed and immediately put "live", without special publication versions.

As programming languages and our understanding of them evolve, we believe that these guides are "work in progress" and will probably never see a status "finished"; as agile developers, we welcome this.

Open Source

This repository is open source, meaning it is written by a loose community of interested persons, and anybody from within and without SAP is invited to contribute.

LICENSE describes how you may use this material. Creative Commons' summary may be easier to understand for non-jurists. In short, you can freely use and share this repository's content. You can also use it in own works, such as presentations or books, even commercially, as long as you give credit to this source, and point out your changes. Detailed information including third-party components and their licensing/copyright information is available via the REUSE tool.

CONTRIBUTING describes how you can contribute. In short, you can use GitHub's standard means to add to this repository, and you have to agree to our license to contribute directly.

We believe that clean code should be discussed freely and openly.

Grassroots Project

These guides are grassroots projects, meaning they were started, and are still driven, by programmers who spend their day coding, and want to get better at it.

We are developers, architects, quality engineers, and consultants, from associates to chief experts, from language creators to tool developers, from S/4HANA to the ABAP language group. We respect all roles, ranks, and units, and welcome any suggestions and improvements.

Optional

Following these style guides is optional, meaning you - or more precisely: your team - can choose whether you want to adhere to it. This applies equally to in-house developers, partners, and customers.

We believe that clean code comes from conviction, not from pressure.

Ecosystem

If you want to learn more about the motivation, the underlying principles and the forming ecosystem of tools, books and related practices around the style guide, you can follow the blog on Clean Code: Writing maintainable, readable and testable code.

styleguides's Issues

Structure with INCLUDE TYPE

is there a discussion about structure definition?

my 2 cents: the documentation says INCLUDE TYPE should not be used:
https://help.sap.com/doc/abapdocu_752_index_htm/7.52/en-US/index.htm?file=abapinclude_type.htm

I do not agree. as I like INCLUDE TYPE struct_type AS Name as it helps to avoid a lot of MOVE-CORRESPONDING later.

CONSTANTS

in the https://github.com/SAP/styleguides/blob/master/clean-abap/CleanABAP.md#make-it-easy-to-spot-meaning section there are 2 CONSTANTS statements,

CONSTANTS some_random_number = 782346.
CONSTANTS memory_limit = 4096.

I'm not sure if this is valid ABAP code? At least it does not work on 750, plus I do not recall having seen the syntax before. Perhaps it works on some 750+ versions?

First check input parameters then continue with function logic

In the section Focus on the happy path or error handling, but not both you propose to split a function into 2 functions - check parameters and logic.

I would like to propose a complementary constructions which is IMHO easier to get:

METHOD append_xs.
  IF input = 0. 
    RAISE EXCEPTION /dirty/sorry_cant_do( ).
  ELSEIF input < 0. 
    RAISE EXCEPTION cx_sy_illegal_argument( ).
  ENDIF.

  DATA(remainder) = input.
  WHILE remainder > 0. 
    result = result && `X`. 
    remainder = remainder - 1. 
  ENDWHILE.
ENDMETHOD.

Usage of static methods

Hi ! Thanks for the document again.

Would like to disagree with "Prefer objects to static classes" or at least in it's current formulation.

Static methods can be seen as "functional programming" approach. Pure functional methods are:

• less prone to bugs as they don't use any state
• more testable
• easier to comprehend

Thus I would actually recommend the opposite - tend to use static methods (more accurately "pure functions") when appropriate. Of course "appropriate" is narrower than "always".

• e.g. as mentioned: public static methods (except factory related) would usually mean that they can be separated in an util class, I agree
• if you need to pass state to static method to work - this is also a subject to make it "normal" (1, maybe 2 state parameters are OK), etc

So I'd at least soften the statement as now it looks "run from static methods" whereas it should be "you should cook them properly" or "... depending on your programming style ... you may ...".

OpenSql: CAST with SUM aggregation

Hey Guys,

is it possible to do sth. like this in sql statement:

SELECT key, CAST( SUM( value AS DEC( 13,2 ) ) ) AS value__long..

I want to avoid an overflow without changing the table field.
Would be nice to do this on Hana in the future, without adding the value on application server in my method. Couldn´t find anything useful yet.

Kind regards
Daniel

recommend lowercase writing

Hi,

I'm glad to see, that even SAP found their way to clean code developer.
Please consider to recommand lowercase typing for abap syntax. It's proven by different studies, that reading text in lowercase is much faster than uppercase. If I find the studies I will update this.
In one SAP documentation I found the explanation, why SAP recommand uppercase:
If code is printed out, it's better to read.
First, who ever printed code???
Second, as many studies already prove, it's easier for humans to read text in lowercase, compared to uppercase. So the explanation is wrong too.
Would be great if this would find the way in the official style guide as well.

Usage of Self-Reference 'me'

I have been using the self-reference 'me' to access instance private methods. I used 'me' to distinguish instance methods from static methods.

As per the clean code guidelines it's mentioned to omit it (cf. Omit the self-reference me when calling an instance method).

How would you suggest calling private static methods?

Ensure that messages remain where-used-capable

What I immediately noticed and would like to bring to attention: For maintainability, it is extremely important to have clean error handling and in case of incidents, being able to identify the source of the error / error message.
With message classes, we have (ever since) the possibility to do this, using the Get-Where-Used-List feature of the system. However, this works if, and only if, the message can be tracked back to its message class.
I noticed the bad habit that people create messages with literals, that cannot be found even with full text search. Clean Code for me here would mean that we keep the relation to the (SYST-)MSGNO.

This issue is a mirror of an issue that was originally created in the SAP-internal predecessor (SAP-internal link) to this Open Source repository.

I think it's still valid and should be added to this guide.

Programming paradigms

I take issue with two subsection titles:

• Prefer object orientation to imperative programming
• Prefer functional to procedural language constructs

because you can do imperative programming in OO and the new syntax is not functional but expression oriented. I think better suited names (meaningful) would be

• Prefer object orientation to procedural programming
• Prefer expressions to statements

The second subsection should be clarified by promoting expressions only for functional code without side-effects.
JNN

Static analysis

Hi,
First of all, thanks, very nice guide and thanks for sharing 👍

Any thoughts on including links/recommendations regarding static analysis tools supporting the guidelines?

Eg.
https://github.com/SAP/styleguides/blob/master/clean-abap/CleanABAP.md#omit-the-optional-keyword-exporting
is implemented by
https://docs.abapopenchecks.org/checks/30/

and some of
https://github.com/SAP/styleguides/blob/master/clean-abap/CleanABAP.md#omit-the-parameter-name-in-single-parameter-calls
is found by
https://docs.abapopenchecks.org/checks/43/

Consider using immutable instead of getter

Hi,
I prefer getter over immutables in cases like this:

is_new_version_active
        RETURNING
          VALUE(rv_is_active) TYPE abap_bool,

In this case, I can use
IF object->is_new_version_active()
vs (for immutables):
IF object->is_new_version_active = ABAP_TRUE

which I think is easier on the eye.

What do you guys think?
PhiL

Do not chain up-front declarations

Hi, I agree with that rule in general. Until I found the auto-alignment feature of both abap editor in SE80 and also in eclipse.
This auto-alignment will align the TYPEs so it looks much cleaner than individual lines. And there is no manual editing necessary.

Additionally, eclipse usually deals pretty well with the chained declarations. So there is typically no need to do manual editing as described in that section.

Wouldn't that be a reason to reconcider that section?

Cheers,
Peter

Split method instead of Boolean input parameter

Splitting the method may simplify the methods' code and describe the different intentions better

update_without_saving( ).
update_and_save( ).

The aboce example is wrong, isn't it. That suggest would implement, that some code will be doubled.
It should refactored to this:

update( ).
save( ).

Use of ASSERT

I agree with the principle of Dump for totally unrecoverable situations

However, I much prefer using ASSERTs. Yet they are not mentioned anywhere in the guide?

IF foo IS INITIAL. 
  MESSAGE x666(general).   "Should never happen!
ENDIF.

vs.

ASSERT foo IS NOT INITIAL. 

I must confess I also (mis?)use ASSERT 1 = 2. to force a dump. I don't use MESSAGE because it belongs to the classic dynpro world and the doco is pretty clear about not using it, and even explicitly advises against the above example:
https://help.sap.com/doc/abapdocu_752_index_htm/7.52/en-US/index.htm?file=abenmessages_guidl.htm

Sometimes I'll use ASSERT only to avoid the compiler complaining about an uncaught exception. There are occasions I want an exception to dump, but to keep the syntax check happy I have to do a clumsy catch-and-dump. A typical use case is:

TRY.
    data(guid) = cl_system_guid=>create_uuid_32_static( ). 
  CATCH cx_uuid_error.
    "We have bigger problems if this failed
    ASSERT 1 = 2.  
ENDTRY.

Thoughts? Should some recommendations on ASSERT be included in the guide?

static exceptions

Thanks for the guide, very thorough and useful.
There's one item I deeply disagree with though: exception handling
throw cx_static_check

I do appreciate the value in documenting what exception a method throws, but then you're left with 3 horrible options

• add the exception to every method who calls it
  • all the way up the calling chain
  • being ready to update every single class/interface that could somehow call it every time you add a new one
• add a catch all like throwing cx_static_check to interrupt the madness, which defeats the purpose
• fill your methods with catch and rethrow

My usual approach with exceptions is:

• ignore all warning about uncaught exceptions
• let them bubble up where I decide to handle them
• assume any method could fail with about any exception
• when I decide to handle them:
  • unwrap cx_sy_no_handler if needed (either to handle the parent or give a meaningful error)
  • handle the expected ones
  • let the unexpected bubble up (or rethrow if embedded in the dreaded no_handler)

I honestly feel this approach is much cleaner than ignoring syntax check warnings or littering dozens of methods with exception information just to allow them to bubble unchanged

MESSAGES in Exception

Hello,
First of all I would like to thank you for this amazing work!
It's becoming the bible for any conscious ABAPER!

I would like to ask you your opinion about passing MESSAGES directly in the RAISE EXCEPTION command.
From 7.50 we know that we can now convert messages into exception using the following:

RAISE EXCEPTION TYPE cx_demo_dyn_t100
MESSAGE ID sy-msgid
TYPE sy-msgty
NUMBER sy-msgno
WITH sy-msgv1 sy-msgv2 sy-msgv3 sy-msgv4.

What would be the best practice here?
My personal problem is that I have several systems and not all of them are in 7.50 so I have to switch strategies depending on the system.

In my opinion, this new form is better because you need fewer lines of code, gets more readable, and we avoid the creation of Message Classes to be used in the TEXT_ID property of the exception class.

Nevertheless, I believe this guide should have some hints on the topic. Do you agree?

Thanks again for your work!
Sérgio

Prefixes in identifiers

I'd like to comment on the suggestions to drop variable and parameter prefixes entirely.
https://github.com/SAP/styleguides/blob/master/clean-abap/CleanABAP.md#avoid-encodings-esp-hungarian-notation-and-prefixes

While I personally like my prefixes just because I got used to them in my opinion this is a valid recommendation that one can decide on to follow. Especially because of the length limitations in ABAP for everything, so you get more space to describe things. I would however argue that if you already have an existing code base that follows some kind of prefixing naming convention (like the SAP one in the code inspector...) it is worse to write new code without prefixes than to just follow your existing guideline. The advantage of having no prefixes at all is very minor (neglectable) compared to having a consistent code base, in my opinion.

And by the way:
Other languages like Java use absolutely no prefixes, and still Java code is perfectly readable.

One notable exception is the Android Open Source Project, which is written in Java and uses prefixes for static and member variables:
https://source.android.com/setup/contribute/code-style#follow-field-naming-conventions

It violates the current Google Java Style Guide

https://google.github.io/styleguide/javaguide.html#s5.1-identifier-names

While the opinion might have changed on this topic they keep their convention because to them consistency is more important:

Our parting thought: BE CONSISTENT. If you're editing code, take a few minutes to look at the surrounding code and determine its style. If that code uses spaces around the if clauses, you should too. If the code comments have little boxes of stars around them, make your comments have little boxes of stars around them too.

The point of having style guidelines is to have a common vocabulary of coding, so people can concentrate on what you're saying, rather than on how you're saying it. We present global style rules here so people know the vocabulary, but local style is also important. If the code you add to a file looks drastically different from the existing code around it, it throws readers out of their rhythm when they go to read it. Try to avoid this.

https://source.android.com/setup/contribute/code-style#be-consistent

(Also since Java accounts for lower and upper case code constants (static final) have the convention of being all upper case and separated by underscore which is sort of a naming convention that just does not compare to ABAP in the same way for obvious reasons.)

Feel free to close this issue, just wanted to comment.

Table accesses and performance: BINARY SEARCH vs. secondary keys

The tables section does not give any guidance on approaches for performance optimised accesses to standard tables, especially in areas where SORTED/HASHED tables are not possible.

The classical approach is to use SORT ... BY ... and later in the code READ ... BINARY SEARCH, which is error-prone/dangerous as all control flows leading to the READ statement need to ensure the sorting order, as well as any modifications of the table. ABAP supports the concept of secondary keys. In particular the non-unique sorted ones are non-intrusive as they sort on demand and also only initialise the sorting order lazily on first usage. Unique and hashed come with performance penalties and also lead to dumps on uniqueness violations.

A common observation is that people often introduced sorted tables, only to later find different uses and had to introduce other sorted tables for these. Thus, sorted tables look like a pattern that should only be used if the sorting forms an inherent aspect of the table itself.

For other cases, especially the usual performance improvements, using a standard table with (probably multiple) secondary keys seems to be preferable, because it is more flexible.

The document Clean ABAP deals with performance only slightly. While it's important to touch the topic, the guide shouldn't become an in-depth exploration of performance topics.

However people are in many cases used to deeply rooted local performance optimisation patterns like sorted tables etc. and these patterns usually are either dangerous or don't feel "natural" in code built along the principles of clean code. Hence it would be good to clearly state preferred approaches here.

I also cannot provide a precise guidance, but maybe it would be a start to start for discussion to begin with a standard table and to introduce a non-unique sorted secondary key in case the table is expected to contain more than 50 records and there is more than log(n) accesses to the table using a certain key combination. Unique sorted and hashed keys are in my opinion only helpful to ensure there are no duplicates.

Maybe someone is up to writing a nice pull-request for this.

Indentation of parameters

Hi! Great document. Clean, searchable, with examples, very good job. Thanks !
I'd say 80% are to "immediate compulsory application" but there are couple of things I disagree (this and a separate issue I'll create in a moment).

I'd strongly disagree with "Keep parameters behind the call", "If you break, indent parameters under the call" and thus indirectly with most of examples in "Formatting sections". The most concentrated example I would definitely avoid is in "Indent in-line declarations like method calls" section - multilevel placement of parameters. This is imho exactly the opposite to "optimize for reading" intention :)

This way of alignment depends on variable/callee length and thus places in unpredictable place and thus directly cripples the readability.

XXXX = XXXXXXXXXXXXXXX ( YYY    = ZZZ 
                         YYYYYY = ZZZZZZZ ).

versus

XXXX = XXXXXXXXXXXXXXX ( 
  YYY    = ZZZ 
  YYYYYY = ZZZZZZZ ).

In the second case eye expects parameters in a familiar place and thus consumes it faster. This way "keeps the important things in a single (left) column"

For the reference: The subject is very well clarified in this video (staring at 10:30 ~ 23:00 with some very bright examples at ~21:00+ ).

P.S. BTW the rest of the mentioned video also worth integrating :) E.g. using "get" in method names.

Open SQL: Obsolete Host Variables

Suggest adding a section corresponding to https://help.sap.com/doc/abapdocu_750_index_htm/7.50/en-US/abenopen_sql_hostvar_obsolete.htm

S/4HANA Style Guide

Maybe it should be mentioned that Clean ABAP is now the official style guide for SAP S/4HANA.

Kind regards,
Knut

prefer enumerations over booleans

maybe ABAP was more modern than other languages in not providing booleans, and with the addition of abap_bool became as worse as others?

There is a debate about using booleans outside, e.g.

http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html
https://silkandspinach.net/2004/07/15/avoid-boolean-parameters/
http://www.beyondcode.org/articles/booleanVariables.html (2003)
Reliable Software Technologies - Ada-Europe 2001: 6th Ada-Europe (https://books.google.de/books?id=E0aPZtAMJBQC&pg=PA50&lpg=PA50&dq=boolean+considered+harmful&source=bl&ots=5eXX4ZIMbS&sig=ACfU3U0sAXNMB3Aa-0Y2lo2OF1ua_mekcA&hl=de&sa=X&ved=2ahUKEwjFvay9uP_hAhUB3KQKHahwBrs4FBDoATADegQICRAB#v=onepage&q=boolean%20considered%20harmful&f=false)

In fact it is really hard to date it back as google prefers newer webpages over older…

Can we please add a section “Don´t use booleans where you don´t know it is binary forever?” to https://github.com/SAP/styleguides/blob/master/clean-abap/CleanABAP.md#booleans ?
Not using booleans but enumerations usually improves readability of code significantly unless the domain is in fact binary.
Cheers, Joachim

Mentioning of Avalon

Dear All,

First thanks for that really usable best-practice style guide. This is really something which was missing in the ABAP world.

One request: In our projects we were still struggling to create test-doubles / identify a usable test-double-framework for AMDPs. In the best-practice style guide you are mentioning the tool "Avalon". Honestly I am not aware of this tool, and a quick search on https://launchpad.support.sap.com, or help.sap.com didn't bring anything up.
Can you provide any link for this tool?

Thanks a lot,
Timo

Consider converting legacy messages into class exceptions

I like to convert all MESSAGEs (coming out of Function Modules or other legacy code) into class exceptions. This way everything is handled in the same way.

In order to accomplish this, my root exceptions are extended with the following functionality:

1. An extra bool attribute called MESSAGE_FROM_SYST
2. A local class which is will hold the SYST message data:

class lcx_t100_syst definition inheriting from cx_dynamic_check final.
  public section.
    interfaces if_t100_message.
    methods constructor.
    data msgv1 type symsgv.
    data msgv2 type symsgv.
    data msgv3 type symsgv.
    data msgv4 type symsgv.
endclass.                    "cx_t100 DEFINITION

class lcx_t100_syst implementation.
  method constructor.
    super->constructor( ).
    me->msgv1 = sy-msgv1.
    me->msgv2 = sy-msgv2.
    me->msgv3 = sy-msgv3.
    me->msgv4 = sy-msgv4.
    if_t100_message~t100key-msgid = sy-msgid.
    if_t100_message~t100key-msgno = sy-msgno.
    if_t100_message~t100key-attr1 = 'MSGV1'.
    if_t100_message~t100key-attr2 = 'MSGV2'.
    if_t100_message~t100key-attr3 = 'MSGV3'.
    if_t100_message~t100key-attr4 = 'MSGV4'.
  endmethod.                    "constructor
endclass.                    "cx_t100 IMPLEMENTATION

3. An extra method BUILD_FROM_SYST() in the ZCX class:

method BUILD_FROM_SYST.
  DATA: o_exp TYPE REF TO lcx_t100_syst.
  IF me->previous IS NOT BOUND AND me->message_from_syst = abap_true.
    TRY .
        RAISE EXCEPTION TYPE lcx_t100_syst.
      CATCH lcx_t100_syst INTO o_exp.
        me->previous = o_exp.
    ENDTRY.
  ENDIF.
endmethod.

4. Add an enhancement to the CONSTRUCTOR so that it calls BUILD_FROM_SYST():

method CONSTRUCTOR.
(...)
ENHANCEMENT 1  ZCX_ROOT.    "active version
  build_from_syst( ).
ENDENHANCEMENT.
endmethod.

Maybe this is too convoluted and beyond the scope of this guide. If so please just go head and close it.

Links to blogs

There are some classic blogs, eg https://blogs.sap.com/2016/02/05/fanning-the-flames-prefixing-variableattribute-names/ for https://github.com/SAP/styleguides/blob/master/clean-abap/CleanABAP.md#avoid-encodings-esp-hungarian-notation-and-prefixes that might be worth linking for providing more background and dicussion.

Again, not sure how to format/present this in the document, same issue as #3

"If you don't use enumeration classes, group your constants" last example

Hi! Great work you've done here!

I've spotted an error in the last code example of section "If you don't use enumeration classes, group your constants", where the closing sentence should be ENDDO instead of ENDWHILE.

However, I'd like to suggest changing this example for the following, correcting the mistake but also not needing to define a variable with the total of possible values (which should be updated every time we add a new one):

DO.
  ASSIGN COMPONENT sy-index OF STRUCTURE message_severity TO FIELD-SYMBOL(<constant>).
  IF sy-subrc IS INITIAL.
    IF input = <constant>.
      DATA(is_valid) = abap_true.
      RETURN.
    ENDIF.
  ELSE.
    RETURN.
  ENDIF.
ENDDO.

Best regards,
Martín

Slack access/invitations

https://github.com/SAP/styleguides/blob/master/CONTRIBUTING.md
mentions
"
Join our Slack workspace sapcodestyleguides.slack.com if you want to discuss guidelines or possible changes without actually submitting anything.
"
but going to the page it says
"Don't have an account on this workspace yet?
Contact the workspace administrator for an invitation"

how to get an invitation?

CHECK vs. RETURN

The extended syntax check will disagree with me, but I favor the pattern where CHECK is used after the default return value is set.

METHOD read_customizing.
  result = default_value.
  CHECK keys IS NOT INITIAL.
  " do whatever needs doing
  return = new_value.
ENDMETHOD.

I open this issue because it is not discussed here:
https://github.com/SAP/styleguides/blob/master/clean-abap/CleanABAP.md#check-vs-return

JNN

NOT ... IS VS IS NOT

This is a minor topic that I haven't seen covered in the guide.

Isn't it more readable?
IF variable IS NOT INITIAL.

than,
IF NOT variable IS INITIAL.

I have always found the second example more confusing and less related to the natural language. What do you think?
By the way, congratulations for this guide, it is awesome.

Add section: Proper use of Packages

I've tried to find some information regarding the proper use of packages, but it seems packages are barely even mentioned.

Would it make sense to add a section on how to properly use packages?
While there are similarities to classical "package-systems" like in Java, there are a few peculiarities that could be expanded upon.

Local classes are good

I wholeheartedly disagree with the point that local classes should be avoided:
Global by default, local only in exceptional cases

It's taken me so long to raise this issue because I have a lot of thoughts on this and was debating whether to post a blog on SDN to get a wider audience's input. Well let's just get the ball rolling here and see what folks think.

To me locals embody the principle of only making things as public as they need to be. If a class is global it is effectively published and has an API and possibly other consumers that I now need to be mindful of. I see this as detrimental if a class is only responsible for functions of its enclosing class / report / function group.

I also disagree with most reasons given in the guide:

• Local classes hinder reuse because they cannot be used elsewhere.

On the one hand that's often the whole point! But on the other hand the statement is not correct because it is still possible to use (as in: consume) them elsewhere. In fact, you can even use them to create closures - whether this is a good idea is a different discussion :)

• Although they are easy to extract, people will usually fail to even find them.

Maybe it's just me, but many of my local classes grow from private methods that have become too complex or uncohesive. As such I don't see how functionality would be more or less 'findable' in a private method vs. a local class. And from a personal viewpoint, the search tool I use most is where-used, and it will find a local class that deals with table X just fine.

• A clear indication that a local class should be made global is if you have the urge to write tests for it.

Actually that's my exact criteria for extracting a private method into a new (local) class, I think this is also in either Clean Code or one of those books.

• A local class is a natural private cogwheel in its greater global class that you will usually not test.

Yes and no. A local class can be a simple utility class, or it can embody a lot of testable functionality that is completely private to the enclosing program/class/FM.

• The need for tests indicates that the class is independent from its surrounding and so complex that it should become an object of its own.

To me a local class is an object of its own. The only difference is that other classes have no business with it. And the author/owner retains the freedom to change and refactor at will.

I know one counter argument might be package encapsulation. But in my experience, only a small minority of customers use package checks, it's just too much effort to untangle the existing setup to be able to start using them. So it's a nice idea but mostly theoretical.

Prefer inline to up-front declarations

I personal fully agree with this, but maybe we can add a comment like "to not break the logic of complex legacy code". If you add in the middle of a 800 lines spaghetti code function suddenly inline declaration it might getting more and more unreadable.

Avoid encodings, esp. Hungarian notation and prefixes

This might be related to #48 but since that is closed with #51 I'd like to take the chance to describe some problem I had with omitting prefixes, especially when it comes to (local) types.

Consider I have a class with a local type and want to have a member attribute with the same name. This isn't possible without any differentiation between type and variable.

CLASS zcl_tms_transport DEFINITION
  PUBLIC
  CREATE PUBLIC .

  PUBLIC SECTION.
    " ...
  PRIVATE SECTION.
    TYPES:
      BEGIN OF transport_change,
        change_request_key         TYPE key,
        transport_request          TYPE trkorr,
        date_quality_assurance     TYPE lxeexphead_impdate,
    END OF transport_change.
    TYPES:
      transport_changes TYPE SORTED TABLE OF
        transport_change WITH UNIQUE KEY change_request_key.

    DATA transport_changes TYPE transport_changes . "<-- This line results in syntax check error
    " ...
ENDCLASS.

In "old Hungarian notation" I would've written this like so

CLASS zcl_tms_transport DEFINITION
  PUBLIC
  CREATE PUBLIC .

  PUBLIC SECTION.
    " ...
  PRIVATE SECTION.
    TYPES:
      BEGIN OF mst_transport_change,
        change_request_key         TYPE key,
        transport_request          TYPE trkorr,
        date_quality_assurance     TYPE lxeexphead_impdate,
    END OF mst_transport_change.
    TYPES:
      mtt_transport_change TYPE SORTED TABLE OF
        mst_transport_change WITH UNIQUE KEY change_request_key.

    DATA mt_transport_change TYPE mtt_transport_change . "<-- This line is fine
    " ...
ENDCLASS.

Cheers
Jens

Wrong Link in section "if your global class is create private leave the constructor public"

The link to help.sap.com in the section https://github.com/SAP/styleguides/blob/master/clean-abap/CleanABAP.md#if-your-global-class-is-create-private-leave-the-constructor-public
references a SAP internal site.

Should be exchanged to official help.sap.com site

Omit the parameter name in single parameter calls

Omit the parameter name in single parameter calls

DATA(unique_list) = remove_duplicates( list ). 

instead of the needlessly longer

" anti-pattern 
DATA(unique_list) = remove_duplicates( list = list ). 

There are cases, however, where the method name alone is not clear enough and repeating the parameter name may further understandability:

car->drive( speed = 50 ). 

Maybe this question is related to optional parameters, which also were recommended to omit. We have cases like:
Function module used across many teams, when change of parameters causes dumps on all the places, where it is used. Then there is a possibility to maintain it just by adding OPTIONAL parameters.
Now back to this recommendation – if it were opposite (use always name of all the parameters), then no adaptation of method callers is needed. That is why I would say “use the parameter names always, not never”. But I would ask, what do you recommend about maintaining of widely used function module and optional parameters. How to solve situation as above?
PS: pity that we have no chance to overload the method. I would say optional parameters are in ABAP to compensate it.
Together with this, DEFAULT parameters would also causes the same problems as OPTIONAL ones, since they can be skipped within function call. Are those rules also valid for DEFAULT?
Then, we have keyword PREFFERED, which could also help. But I feel the simplest is simply to state the parameter names. It seems to me no deal.

Recommended way to handle errors in mass operations

Hi colleagues,

your guidelines are very helpful so far. But I could not find any recommendation for error handling during mass operations. How should an API tell the caller, that one of many operations failed (and why it fails)?
For example if I implement a method, which adds multiple items to a shopping card: In this case it make sense to continue adding the other items although adding one item may fail. The caller of this API is probably wants to know, which items failed and why to give feedback to the end user (or do other stuff).

Exporting an additional message table works, but is not really nice in my opinion. Do you have any suggestions for this?

Thanks and best regards,
Heiko

Prefer FIELD-SYMBOLS inside of loops

From an SAP developer:

For LOOPs, FIELD-SYMBOLS are the better choice and faster - unless the tables are not already references themselves.
There is also an ATC check for this.

Therefore, the section https://github.com/SAP/styleguides/blob/master/clean-abap/CleanABAP.md#prefer-ref-to-to-field-symbol should rather be something like:
Prefer REF outside of loops
Prefer FIELD-SYMBOLS inside of loops

See also the example from the ABAP docu.

Replace internal links

Hi,

there seem to be some SAP internal links in the document. Could these be replaced with public ones?
https://github.com/SAP/styleguides/search?q=sap.corp&unscoped_q=sap.corp

Exception-class structure

Hi,
First of all thanks for all your work on this valuable source of inspiration.

I am curious about the consensus you reached on the structure of common exceptions (lock error, not found in BD...) would you care to share more on the topic?

You use cx_sy_illegal_argument in some example (every method with input parameter could raise this one). Do you use an unckecked global exception or do you a subclass a more generic - checked - exception (zcx_prodord_illegal_argument)?

Thanks & Regards,
Alexandre

New suggestion: method header comment

I have found it useful to add a simple header comment to all methods, to explain the purpose of the method and also to keep a change log for the method.

The change log includes

• a reference to some kind of external change document, JIRA issue, etc
• the user name of the developer
• the date of the change
• a short description of the purpose of the change.

The purpose of the header comment is two fold:

• Sometimes we cannot adequately express the purpose of a methods with the limited number of characters available for an ABAP method name. The method header allows us to elaborate on the method name.
• SAP and ABAP does not (yet) have proper support for Git or other version control systems. The change log comment can been seen as an alternative to the git commit message - a short explanation of the purpose of the change, which will be immediately be visible when comparing the current version of the method with the previous version in for instance Eclipse.

Is this something you would consider adding to the style guide?

Example of header comment:

METHOD append_xs.
*-----------------------------------------------------------------------
* Add Xs to a string
*-----------------------------------------------------------------------
* Change     Developer    Date       Description
* CHG0155570 USAP         01.07.2018 Method created
* CHG0155587 FSTY         05.01.2019 Refactoring to use inline variables
*-----------------------------------------------------------------------  
  
  validate( input ).
  DATA(remainder) = input.
  WHILE remainder > 0.
    result = result && `X`.
    remainder = remainder - 1.
  ENDWHILE.
ENDMETHOD.

Interfaces vs. abstract classes

Received this question as an e-mail:

This is regarding the section Public instance methods should be part of an interface.

I couldn't find any mentioning of abstract classes as alternative to class interfaces in the document. I have good experiences with abstract classes and inheritance and don't see that public methods must be part of the interface, as stated here. What are the arguments against using abstract classes instead of interfaces?

Passing value type importing parameters

First of all, thank you for this awesome guide, really appreciate the work that has been put into it.

I didn't find a recommendation on when to use pass by value/reference in case of importing parameters. This is probably a minor point but maybe worth mentioning because in other programming languages, the default behavior is that value types (int, struct, double etc.) are passed to methods by value (ie. they are copied). I think many developers expect this behavior subconsciously in ABAP too, however everything is passed by reference unless VALUE( ) is explicitly specified.

This can create some weird unexpected side effects for example when you pass sy-tabix to a method as an importing parameter (might be an anti-pattern itself...?) and there is a LOOP or READ TABLE inside the method too. There are probably other similar examples.

Function Groups vs Classes

Received this input from internally. Translation by me.

Related to the sub-section Function Groups vs. Classes.

No substitution.
A call to a function module is technically always dynamic.
Therefore the syntax check for the signature is also only done at runtime.
This means it is always possible to put the function module name into a variable and call it dynamically.
This is also what frameworks like BDT and BRF+ do.
The sentence in the guide's section thus is wrong.

Variable encapsulation.
There is no real privacy for variables of function groups.
They can even be modified from the outside.
This statement thus is plainly wrong.
For example:

FIELD-SYMBOLS <fs>TYPE any.
ASSIGN ('(<report_name>)gv_global_variable') TO <fs>

Method encapsulation.
This statement too is simply and completely wrong.
Form routines can always be called from the outside.
For example:

PERFORM set_buffer_true IN PROGRAM <some_program>.

chained data declarations contradiction

Hi there!

these two sections are inconsistent with one another:

1. do not chain-up front declarations
2. Optimize for reading, not for writing

In the first section you declare chaining-up data declarations as bad code.
In the 2nd section you say, one should prefer colon separated data declarations.

Due to refactoring issues I am absolutely with the recommendation to use single data statements for each declaration.

Maybe we should find a better example for readable code?!

Cheers
Enno

Consider splitting into multiple files

Hi,
The Markdown file is current 4400+ lines, have you considered splitting this into multiple files? This might make it easier to read the change history plus decreases the possibility for merge conflicts.

GitHub offers Jekyll for static site analysis so its possible to present the guide in a format different than the file structure, https://help.github.com/en/articles/using-jekyll-as-a-static-site-generator-with-github-pages

use-own-super-classes for special text handling

https://github.com/SAP/styleguides/blob/master/clean-abap/CleanABAP.md#use-own-super-classes

Context:
[...]. Enables you to add common functionality to all exceptions, such as special text handling.[...]

Hi. If I use
Raise Exception Type xxx Message ID...
I have no way to find out what message ID has been used inside the constructor of the exception (or the super-constructor). But that is how I understood your suggestion around "special text handling" .. I wanted to log that away...

Am I right and access to the message ID is impossible?
How did you mean the text otherwise?

Thanks!
PhiL

Headline "Test interfaces, not classes" is misleading

Proposal: "Test code under test via interface"

Golden Rules: example for functional constructs does not work in ABAP

The example for functional constructs ("index += 1") does not work in ABAP. You will still have to use "index = index + 1" or "ADD 1 TO index"...

"Use unchecked exceptions"

Following the recommendation of Clean Code to "use unchecked exceptions", I ALWAYS end up using CX_NO_CHECK as my custom exceptions' superclass. I think CX_STATIC_CHECK adds too much noise everywhere it's used. CX_STATIC_CHECK is just like using checked exceptions in Java, it forces people to add empty catch blocks, deal with exceptions that they have no idea how to deal with, or to propagate exceptions in their method signatures needlessly. CX_NO_CHECK is the closest to unchecked exceptions. (CX_DYNAMIC_CHECK is just weird.)

I feel that the recommendation in this guide is moving contrary to years of experience with checked/unchecked exceptions, at least in the Java community, distilled in Clean Code. What do you think?

Drop namespace prefixes - /clean/ /dirty/ etc.

The examples this guide often use prefixes /clean/ and /dirty/ to indicate good and bad examples.

Personally I'm not a fan of their use. To me they are actually an anti-pattern, as they add words and symbols to the examples that decrease readability without adding new information. It's already clear from the context and/or comments what type of example it is, so I find them redundant.
Use of this type of convention also make it more difficult to express finer nuances as you can with comments, such as "use only when necessary", or "inferior".

Take for example, the Interface Pattern sample code:

" inferior pattern
INTERFACE /dirty/message_severity.
  CONSTANTS:
    warning TYPE symsgty VALUE 'W',
    error   TYPE symsgty VALUE 'E'.
ENDINTERFACE.

I would prefer to see this written without /dirty/:

" inferior pattern
INTERFACE message_severity.
  CONSTANTS:
    warning TYPE symsgty VALUE 'W',
    error   TYPE symsgty VALUE 'E'.
ENDINTERFACE.

What do people think?