SAP / styleguides

This repository provides SAP style guides for coding and coding-related topics.
Other
1.68k stars 446 forks source link

Method Ordering #221

Open pokrakam opened 3 years ago

pokrakam commented 3 years ago

Clean Code recommends ordering methods by importance and call sequence. I find this helpful and do it where it makes sense.

It would be nice to incorporate this into the Style Guides, and Eclipse and SE80's source-based editor let you put methods in order of your choosing. However SE80's form based editor will always reorder method implementations in alphabetical sequence, even contrary to any reordering one may do on the methods tab.

Though it's not the most important issue in the world, it does cause some issues in Git https://github.com/abapGit/abapGit/issues/2321

So for example you can arrange your methods neatly in top-to-bottom readable order:

CLASS zcl_foo DEFINITION. 
  PUBLIC SECTION. 
    METHODS get_some_value RETURNING VALUE(result) TYPE i.
  PRIVATE SECTION. 
    METHODS calculate_it RETURNING VALUE(result) TYPE i.
ENDCLASS. 

CLASS zcl_foo IMPLEMENTATION. 

  METHODS get_some_value.
    result = calculate_it( ).
  ENDMETHOD.

  METHODS calculate_it.
    result = cl_abap_random_int=>create( )->get_next( ).
  ENDMETHOD.

ENDCLASS.

But then someone using SE80's form based editor will always move calculate_it to the top.

Is it worth including this in the style guides with a caveat? Or does someone have the right connections to get SE80 "fixed"?

Reference: Clean Code Ch. 5 Formatting / Vertical Ordering: In general we want function call dependencies to point in the downward direction. That is, a function that is called should be below a function that does the calling. This creates a nice flow down the source code module from high level to low level. ... Martin, Robert C.. The Robert C. Martin Clean Code Collection (Collection) (Robert C. Martin Series) . Pearson Education. Kindle Edition.

fabianlupa commented 3 years ago

Note the alphabetical method reordering also sometimes happens in ADT at random and it also happened to me after a system copy.

HrFlorianHoffmann commented 3 years ago

Three years back I've reached out to ask whether the automatic method reordering could be changed. I was told that the form-based class editor is in maintenance mode, meaning it only receives fixes for severe bugs and security issues - and method reordering was considered neither.

Now that Clean ABAP is the standard guide for S/4HANA and tools like abapGit, Code Pal, etc. have gained momentum, we could try asking nicely again. I'll reach out to the colleagues and see what I can do.

As long as members of a team use this editor, reordering methods by hand is a somewhat frustrating task. 😯 This is one reason why we never addressed vertical ordering in Clean ABAP.

An optimal solution from my point of view would be a powerful source code formatter that also takes care of the method ordering. For example, public > protected > private, with "calls" arrows pointing only from top to bottom, and some default ordering - e.g. alphabetical - where it doesn't matter. This would ensure readability and ensure unique look of the code in all systems.

To this date I do not know why the alphabetical reordering sometimes appears in ADT as well, by the way. I tried to debug it once but couldn't pin it. If you are affected, an easy means is to hit Ctrl+Z immediately to undo your changes, then Ctrl+Y (is that it?) to redo them, then save again. As it's a seemingly random issue, chances are that the repeat save doesn't run into it again.

pokrakam commented 3 years ago

Thanks @HrFlorianHoffmann, I think with the advent of gCTS the same problem we are seeing in abapGit is likely to surface in SAP's own tooling - i.e. changing a line of code in SE80 triggers a delta across the whole class. Although technically not a bug, perhaps the gCTS angle could help push it into a category of severe impairment 😄

larshp commented 3 years ago

well in https://github.com/stubbini/CEI_GCTS/tree/main/objects/CLAS/ZIMS1 each method is stored in a separate file(like the internal database representation) ie. not a problem for gCTS

jordao76 commented 3 years ago

An optimal solution from my point of view would be a powerful source code formatter that also takes care of the method ordering. For example, public > protected > private, with "calls" arrows pointing only from top to bottom, and some default ordering - e.g. alphabetical - where it doesn't matter. This would ensure readability and ensure unique look of the code in all systems.

If a formatter could do that, then it could also help with analyzing the class cohesion. There could be islands of methods that don't "talk" to other methods, and that would help to refactor and split the class into different concerns, further improving the design.

However, I don't usually agree with a formatter, and so at least we'd have a good tool to visualize (perhaps in a nice graph) methods and attributes relationships. It could go even further to classes relationships and then we would have all the nice metrics defined by Robert Martin like afferent/efferent coupling, instability and abstractedness.

larshp commented 3 years ago

yea, something like https://steampunk.abaplint.org/CLAS_CL_ABAP_C_WRITER.html ?

mbtools commented 3 years ago

Understandable that SAP does not want to touch legacy code. However, there's clearly a need to automate code formatting beyond the SAP standard. The logical conclusion is to provide an enhancement spot in the "post pretty-print" location. It shall work in SE24/80 as well as ADT, of course.

ConjuringCoffee commented 2 years ago

This problem still persists with gCTS. When imported into a target system, the order of the method implementations is not kept. The pattern by which the method implementations are ordered isn't obvious either. It's not alphabetically and it doesn't depend on the method declarations.

N2oB6n-SAP commented 1 year ago

To sum it up: There are multiple technical reasons why the noble attempt at vertical ordering is almost futile. Fragility is coming from editing tools as well as from transport mechanisms. All of these point to the very hard truth about ABAP which is that vertically ordered code simply has limited chances of survival because of the underlying storage/format. This is a fundamental difference from programming languages where the full textual representation (file-based in most cases) of modules IS the original source.

Personally, I would not veto adding a paragraph to acknowledge the effort yet highlight the futility of vertical ordering in ABAP. Pragmatically speaking, though, we might as well leave it out to keep the guide short considering that we cannot give a clear recommendation here. (In a controlled environment such as a single development system where a module originates from and where all participants refrain from using SE80 it is possible to keep vertical order. If you accept that the benefit of vertical ordering is likely lost once the code leaves that system and you still consider the additional effort worth it you are free to pursue this, of course.)