Feature request: Order method implementations

[ConjuringCoffee]

I'd like to request a new rule to order the method implementations based on the order of the method definitions.

Example:

CLASS lcl_example DEFINITION CREATE PUBLIC.

  PUBLIC SECTION.
    METHODS method_1.
    METHODS method_2.

  PROTECTED SECTION.

  PRIVATE SECTION.

ENDCLASS.

CLASS lcl_example IMPLEMENTATION.
  METHOD method_2.
  ENDMETHOD.

  METHOD method_1.
  ENDMETHOD.
ENDCLASS.

Expected result:

CLASS lcl_example DEFINITION CREATE PUBLIC.

  PUBLIC SECTION.
    METHODS method_1.
    METHODS method_2.

  PROTECTED SECTION.

  PRIVATE SECTION.

ENDCLASS.

CLASS lcl_example IMPLEMENTATION.
  METHOD method_1.
  ENDMETHOD.

  METHOD method_2.
  ENDMETHOD.
ENDCLASS.

[jmgrassau]

Hi ConjuringCoffee,

this would of course make the comparison with previous revisions rather difficult to read (so, surely an opt-in rule that is deactivated by default), but it does make sense – you could quickly change the order of METHODS definitions in the class definition and then, with one keystroke, align the implementations accordingly.

However, two concerns:

• ABAP cleaner usually can't "see" the interface definitions implemented by a class (unless they are local interfaces), so it cannot order the interface method implementations. (So probably, you would put those at the beginning before the public non-interface methods, and just keep the existing order)
• More importantly, for non-interface methods, this would inevitably order the methods implementations by visibility – public methods first, then protected methods, finally private methods. However, if public METHOD_1 uses a couple of private helper methods METHOD_1A, METHOD_1B etc., it would IMHO be better from clean code perspective to have those helper method directly below METHOD_1, not below public METHOD_5.
• So, an alternative idea would be to only reorder the public methods, and then put the private methods below them in the order in which they are first called when reading the code (recursively, since private methods may call other private methods etc.).

Kind regards, Jörg-Michael

[ConjuringCoffee]

this would of course make the comparison with previous revisions rather difficult to read

You're right, I didn't consider older system releases: On newer releases you can use a "Sorted Class Methods" comparison in the revision history to solve this problem.

So, an alternative idea would be to only reorder the public methods, and then put the private methods below them in the order in which they are first called when reading the code (recursively, since private methods may call other private methods etc.).

This is an interesting idea, but I honestly don't know how I feel about it yet.

Another concern is the user's current position in the editor: Are you able to scroll to the method the user was previously viewing before? I think this will create confusion otherwise. Imagine you use the ABAP Cleaner and suddenly your method is gone, and you have to go looking for it... I think this would be quite annoying. This would be especially true with your alternative idea because a single new method call can cause the entire class to be reordered.

[jmgrassau]

Hi ConjuringCoffee,

ah, you're right, with "ABAP Compare (Sorted Class Methods)" this wouldn't be such an issue, and I think this comparison option is available in ADT regardless of the release you are working on (however, it doesn't seem to be available for local classes of which you could have several in the same code document).

Another concern is the user's current position in the editor

I think such a cleanup rule should anyway only work if you select the whole code document (to include both the definition and the implementation part of a class), so changes would not happen if you just press Ctrl+4 inside a method. You definitely wouldn't want code to be jumping back and forth when you just comment out a method call :-)

Actually, this idea is more or less from Robert C. Martin's Clean Code book (chapter 5: Formatting, section "Vertical Distance", paragraph "Dependent Functions" = page 82 in the 2009 edition), where he says:

If one function calls another, they should be vertically close, and the caller should be above the callee, if at all possible. This gives the program a natural flow. If the convention is followed reliably, readers will be able to trust that function definitions will follow shortly after their use.

I don't think this is already a requirement mentioned in the Clean ABAP style guide, but it makes sense to me, esp. for (protected and) private methods.

Kind regards, Jörg-Michael

[ConjuringCoffee]

I think such a cleanup rule should anyway only work if you select the whole code document (to include both the definition and the implementation part of a class), so changes would not happen if you just press Ctrl+4 inside a method.

That makes sense!

I think this comparison option is available in ADT regardless of the release you are working on

Unfortunately, this doesn't seem to be the case: Using the same ADT it is available in our 7.57 system, but not in our 7.50 system.

[bjoern-jueliger-sap]

Just a couple of remarks, not intended as definite arguments against the idea:

1. While it is correct that "ABAP Compare (Sorted Class Methods)" makes changes to the method order invisible in the ADT compare, development models using abapGit cannot take advantage of this - the git diff algorithms know nothing about ABAP and sorting its methods!

2. It is not clear to me that the "correct" method order should be inferred from the order of the definitions. This seems to assume that people think carefully about the order of the methods in the declaration sections but essentially place the implementations at random, but is this actually the case?

   For instance, there are possible workflows that generate the method definition completely automatically via quick fixes: You can create a method definition by positioning the cursor on the first line of a method implementation statement and requesting quick fixes, and you can add parameters by positioning it on undeclared variables in a method implementation. In such cases, the definition has not been handwritten and the "order" in which the definition statements appear is more or less random and should not be taken as the order in which the implementations should be sorted.

   Since the benefit of this feature therefore depends on specific assumptions about how users declare methods, I'm not sure this is the kind of rule ABAP Cleaner wants to implement.

[ConjuringCoffee]

It seems like the rule is much more situational than I initially thought.

I think my main use case isn't even with global classes, but when writing unit tests: I like to declare the test methods in a sensible order. First the setup method, then the various test methods. For example, I might order the tests by first having the "happy path" tests, then the "exception" tests. Ordering the test implementations is helpful then, but I mostly don't do it because it is additional effort.

How do others feel about this topic? Maybe it makes sense to restrict the suggested rule to the unit test include?

[jmgrassau]

Hi ConjuringCoffee,

that sounds like a good starting point to me, because as far as I know, FOR TESTING methods are always private, so the question of how their "vertical position" should relate to other (protected or public) methods is not so critical.

Kind regards, Jörg-Michael

[fabianlupa]

To me the testing use case is not as important as the test classes will usually be local classes where the implementations aren't automatically sorted alphabetically by someone using the SE24 form based editor or after a dev system rebuild by system copy from prd.

FOR TESTING methods are always public to my knowledge.

[ConjuringCoffee]

FOR TESTING methods are always public to my knowledge.

See the SAP documentation:

Test methods should be private, or protected if the methods are inherited. Since test classes implicitly offer friendship to the test driver in the runtime environment, the driver can call them. The test methods of a test must only be public in the rare cases where this test is to execute the tests of other test classes.

[ConjuringCoffee]

Related discussion: https://github.com/SAP/styleguides/issues/221