Technical Articles

Jacek Wozniczak

October 7, 2020 3 minute read

A tale of code

Take a look at this:

Mr. Enfield returned: "But I happen to have noticed his address; 
                      he lives in some square or other".

"And you never asked about the—place with the door?" 

Mr. Enfield's reply was:  "No, sir; 
                          I had a delicacy. 
                          I feel very strongly about putting questions; 
                          it partakes too much of the style of the day of judgment. 
                          You start a question, and it’s like starting a stone. 
                          No sir, I make it a rule of mine: 
                            - the more it looks like Queer Street, the less I ask".

The lawyer said: "A very good rule, too".

Mr. Enfield continued: "But I have studied the place for myself".

The pair walked on again for a while in silence and then: "Enfield... 
                                                           That’s a good rule of yours."

And this one

Mr. Enfield returned: 
  "But I happen to have noticed his address; 
  he lives in some square or other."

"And you never asked about the—place with the door?"  

Mr. Enfield's reply was:
  "No, sir; 
  I had a delicacy. 
  I feel very strongly about putting questions; 
  it partakes too much of the style of the day of judgment. 
  You start a question, and it’s like starting a stone. 
  No sir, I make it a rule of mine: 
    - the more it looks like Queer Street, the less I ask."

The lawyer said: "A very good rule, too". 

Mr. Enfield continued: "But I have studied the place for myself". 

The pair walked on again for a while in silence and then: 
  "Enfield... 
  That’s a good rule of yours."

This one:

vs this one:

The flow:

and

Now with code.

Don’t focus on code semantics, but the visual representation. It is just dummy example, it’s all about formatting.

The first examples, following Clean ABAP preferred rules:

and with helper variables:

Now applying refactoring – Alt+Shift+R in Eclipse, some names changed:

the second with helper variables after name changes:

Now different style examples with line break and incremental indentation:

with helper variables:

Applying refactoring, names changed:

and the second version with helper variables refactored:

The last two images shows why I personally prefer using incremental indentation – no messed up code after renaming. Both styles are readable for me but this the second one without so strict alignment is refactoring-friendly, which is a very important thing for constant improvement of code and naming things correctly. I don’t want to manually correct main code + unit test code + dependent code after each name change (and be sure that everyone in a team remember to “fix” code after his or her refactoring changes). This heavily right-aligned code is also hard to read on smaller screens in editors or webpages like version control systems.

Speaking of version control – here are diffs for the Clean ABAP preferred rule, after refactoring I have aligned all things again.

ABAP Git diff (where are my name changes?):

Bitbucket diff:

Now incrementally indented code – changes are visible clearly.

ABAP Git diff

Bitbucket diff:

Interesting video about this topic by Kevlin Henney (starting around 10-11 minute).

The key point from this presentation to me is that keeping the codebase stable after rather trivial refactoring operation is not conforming to a style, but rather to an invariant, a property.

There is also a discussion on this topic in Clean ABAP repo:

https://github.com/SAP/styleguides/issues/21

What are your thoughts, preferences?

10 Comments

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

Mike Pokraka

October 7, 2020 at 12:02 pm

Nice to see this explained so visually. I'm happy to line break for parameters for anything beyond the simplest statements, and if it breaks then keep the parameters under the call.

Personally I prefer to add at least one extra indent if I have a continuation. It's just my preference, I don't know at first glance if a regular 2 space indent is a continuation or a new, subordinate, statement. A bigger indent makes it clear that it's part of the previous statement.

Also, Clean ABAP does advocate not having so many parameters in first place. Consider the parameter skip_unpaid = true: apart from raising a slightly confusing "is it a double negative" question, it also implies two different behaviours of the method. Better to have get_paid_products and get_all_products.

I don't mean to pick apart your example, I realise that you just pulled it out from somewhere, but I wanted to show that it's almost always feasible to make code more readable by removing parameters, especially the 'flag' ones.

Jacek Wozniczak

Blog Post Author

October 7, 2020 at 4:26 pm

Thanks for the comment and sharing your personal preferences!

Regarding

Also, Clean ABAP does advocate not having so many parameters in first place.

But it is not related with the fact, that we have to deal with methods with 2 or more parameters (besides, these methods have 2 parameters, still not "many" for me). Another thing is that if you use expression-style ABAP you can have just one structured parameter in multiple lines.

I've updated the code to be more abstract, the focus here is about formatting and its implications.

Best Regards,

Jacek

Mike Pokraka

October 7, 2020 at 5:33 pm

I didn't mean to suggest you should change the examples! I thought they were very good, I wanted to make the point that the two go hand in hand. When formatting a convoluted statement it's always a good thing to revisit the idea whether that complexity is necessary.

For example it is also easy to get carried away with complex inline constructors and things. My view is that it must not only be easy to read but also debuggable. If there's a complex REDUCE inside a COND I'd rather take it out and assign it to an intermediate variable otherwise it's not possible see the result of REDUCE in the debugger.

My point is that thinking about formatting should also trigger the question whether the code can be simplified. Personally I find I sometimes end up changing the code instead of the formatting, so to me I find it a useful habit 🙂

Jacek Wozniczak

Blog Post Author

October 10, 2020 at 5:14 pm

Hi Mike Pokraka

I didn’t mean to suggest you should change the examples!

I know, but honestly my very first idea to use very abstract code, but I had a EPM model opened (this one from SAP trial servers) and just ran with this products stuff...But now I'm very satisfied with examples 🙂

About this inline statements - I full agree. I really appreciate that ABAP is now more expression-oriented BUT I see the drawbacks - used without self-control code starts to be a nightmare, with 3-4 levels of nesting of constructor expressions. Personally I'm very big fan of introducing auxiliary variables in places, where you could just flow with expressions. But I prefer this for readability and easier debugging, as you are pointing.

My point is that thinking about formatting should also trigger the question whether the code can be simplified.

I fully agree. And the issue here is that sometimes formatting is just easier, you do in on-the-fly, passing by the opportunity to stop and think - "hey, maybe this can be done in other way".

Matthew Billingham

October 11, 2020 at 1:38 pm

I might give it a go.

Felipe Silva

October 11, 2020 at 2:48 pm

Well, might as well leave my 5 cents too.

Great to see people discussing clean code! After all, we are writing it not for ourselves, but for the ones that will work on it after.

What I like to see when I am maintaining code is the second option you gave:

left indented, with lots of auxiliary variables. Chained calls are nice and elegant only if they are kept small, have one single parameter and don’t use construction expressions everywhere:

expect( something )->to_be( 10 )->with_message( 'Expected code score to be 10!' ).

Some people like to look really avant-garde with things like below, making the code one infine chain of constructs and calls…it looks bad, takes longer to understand.

rs_result = new lcl_amount( reduce #( let ... for amount in lt amount where ... next ... ) )->add( reduce #( let ... for amount in lt amount where ... next ...) )->substract( reduce #( let ... for amount in lt amount where ... next ...) )->total( ).

¨it only helps slightly breaking lines (not even sure ABAP supports this that well
rs_result = new lcl_amount( reduce #( let ... for amount in lt amount where ... next ... ) )
->add( reduce #( let ... for amount in lt amount where ... next ...) )
->substract( reduce #( let ... for amount in lt amount where ... next ...) )
->total( ).

Overall I like Grady Booch definition of clean code on uncle Bob Clean Code book:

“Clean code is simple and direct. Clean code reads like well-written prose. Clean code never obscures the designer’s intent but rather is full of crisp abstractions and straightforward lines of control.”

But hey, rather than curse my previous colleague, I do a bit of boy scouting and clean it a bit more, making the code more simple and direct. Digging out the meaning of the logic and putting it inside auxiliaries or full new methods.

data(ls_base) = get_base_amount( ).
data(ls_premium) = get_premium_amount( ).
data(ls_discount) = get_discount_amount( ).
data(lo_amount) = new lcl_amount( ls_base ).
data(ls_result) = lo_amount->add( ls_premium )->subtract( ls_discount )->total( ).

What really annoys me is some people that. apparently, are still working on 1970 mainframes and say “Hey, this is looping three times the same temp table, it should be all inside one loop” and put all inside one giant loop, with a crap switch case to save 1~10 millisecond, proudly.

Regards,

Felipe Silva

Jacek Wozniczak

Blog Post Author

October 19, 2020 at 4:01 pm

Hi Felipe, thanks for your comment.

[...] we are writing it not for ourselves, but for the ones that will work on it after [...]

Like in this famous quote "Programs must be written for people to read, and only incidentally for machines to execute".

Regarding your example of avant-garde - I don't see it in that way. Fluent interface in such chained way are the standard way for me and is heavily used in other languages. I use in Java, Javascript and ABAP as well. It was presented in such way in the original article by Martin Fowler too (https://martinfowler.com/bliki/FluentInterface.html). Lot of examples also in Wikipedia (https://en.wikipedia.org/wiki/Fluent_interface).

I really like helper variables too, but in ABAP such style

cl_abap_testdouble=>configure_call( test_double
  )->ignore_all_parameters(
  )->and_expect(
  )->is_called_once( ).

is fully readable to me and preferred over one-liners.

Best Regards,

Jacek

Felipe Silva

October 19, 2020 at 4:39 pm

Agree 😉 my point was more on avoiding complex constructor expressions (value, corresponding, reduce) while using it. Keeping it simple and clear, such as your example.

Shai Sinai

December 27, 2020 at 1:47 pm

What really annoys me is some people that. apparently, are still working on 1970 mainframes and say “Hey, this is looping three times the same temp table, it should be all inside one loop” and put all inside one giant loop, with a crap switch case to save 1~10 millisecond, proudly.

I guess that, like always, it depends in the use case.

In case you process an internal table with hundreds of thousands of records, it might affect performance.

Joachim Rees

January 21, 2022 at 10:02 am

Just one one point that relay resonates with me:
It would be nice, if after a renaming, formating would not break.

Thanks for that thought!

Technical Articles

A tale of code

The first examples, following Clean ABAP preferred rules:

Now different style examples with line break and incremental indentation:

Assigned Tags