Feature request: Removal of CREATE PUBLIC if not inheriting from another class

[ConjuringCoffee]

Hi Jörg-Michael,

The definition of a class can contain CREATE {PUBLIC|PROTECTED|PRIVATE}.

Of these three possibilities, the use of CREATE PUBLIC is completely unnecessary if the object doesn't inherit from another class.
For some reason, ADT generates a CREATE PUBLIC into every class.

Therefore I'd like to request a rule to remove CREATE PUBLIC from the definition of a class if it doesn't inherit from another class.
To me, it makes more sense to use CREATE PUBLIC explicitly when it is really required.

(It would also be great to know why ADT behaves that way...)

[fabianlupa]

In local classes I also never specify CREATE PUBLIC unless required. For global classes the special situation with the always public constructor comes to mind. But I am not sure if having the addition always there that corresponds to the default value is any help. I personally never bothered removing that generated coding manually (I always just remove the single space between PUBLIC and the dot :) ).

The Clean ABAP style guide also has the CREATE PUBLIC addition for lots of classes where it technically is not required.

[bjoern-jueliger-sap]

I disagree that removing CREATE PUBLIC would be in the spirit of clean code. Shorter code is not a goal in itself, and when the language default contradicts what "should" be the default in the sense of clean code, it is better to never rely on this default behavior at all.

In particular in light of the idea around using interfaces for almost all public methods, I would suggest that classes should, in a "clean" environment, almost always be CREATE PRIVATE and have a static construction method (GET_INSTANCE or CREATE or whatever you like) whose returning parameter is typed as an interface, not as the class itself.

So, if ABAP defaulted to private instance creation with nothing was specified, I would support the idea of omitting the specification CREATE PRIVATE. But ABAP defaults to public instance creation, and I think there is value in the code explicitly documenting this via CREATE PUBLIC. If we agree that classes really should be privately created by default, then it is valuable that users can explicitly see that the class is public instead of having to actively recall the ABAP default, as this might cause them to reconsider if the class really should be CREATE PUBLIC.

[jmgrassau]

Hi ConjuringCoffee, Fabian and Björn,

I think Björn has quite a good point here – and additionally, I wouldn't bet on every ABAP developer knowing what the default is… (and whether it is the same for public and local classes, parent and child classes etc.)

Similarly, if you write TYPES a. you could also hope that everyone knows that a will be TYPE c LENGTH 1, but this is considered obsolete syntax and fixed by the "Make implicit types explicit" rule.

Kind regards,
Jörg-Michael

[fabianlupa]

I don't think every class with public methods needs an interface. Model classes or result objects as value holders are perfectly fine without them for example.
But anyhow, following that reasoning a rule would still make sense, just doing the opposite, adding CREATE PUBLIC where it's missing :P

[ConjuringCoffee]

In particular in light of the idea around using interfaces for almost all public methods, I would suggest that classes should, in a "clean" environment, almost always be CREATE PRIVATE and have a static construction method (GET_INSTANCE or CREATE or whatever you like) whose returning parameter is typed as an interface, not as the class itself.

Thank you, that was a very insightful comment. I never used factory methods because having it typed to the class is bad for inheritance. For some reason, I have never considered using factory methods typed to the interface, not typed to the class?! I'm actually having a small existential crisis about this because this solves a few other issues I've previously had... This is a really good idea and I'll be using this approach going forward.

The other arguments are good too. I'm withdrawing the feature request.

I'll continue the discussion about factory methods in this issue: SAP/styleguides#341

[jmgrassau]

Hi ConjuringCoffee,

I absolutely like that you care about this so much that it puts you to a bit of an existential crisis :-)

Personally, I'd have one exception here: In case of test doubles, I find it quite handy to type their factory methods to the test double class itself (not to an interface), because this way, the caller (i.e. the test method that prepares and injects this double) can directly make use of the test double's helper methods (e.g. to ensure that the test double was indeed called).

What I mean is this: Imagine you have to write a test for some method that uses(!) a calculator class with the following interface (product code):

INTERFACE if_calculator.
  METHODS increase
    IMPORTING iv_number        TYPE i
    RETURNING VALUE(rv_result) TYPE i
    RAISING   cx_sy_arithmetic_overflow.

  " and more methods for fancy calculation capabilities! 
ENDINTERFACE.

Then the test double could look like this:

CLASS ltd_calculator DEFINITION FINAL CREATE PUBLIC FOR TESTING.
  PUBLIC SECTION.
    INTERFACES if_calculator PARTIALLY IMPLEMENTED.

    CLASS-METHODS create_double
      IMPORTING iv_exp_number           TYPE i
                iv_result               TYPE i
      RETURNING VALUE(ro_td_calculator) TYPE REF TO ltd_calculator.

    CLASS-METHODS create_for_exception
      IMPORTING iv_exp_number           TYPE i DEFAULT 2147483647
      RETURNING VALUE(ro_td_calculator) TYPE REF TO ltd_calculator.

    " helper methods for assertions
    METHODS was_called
      RETURNING VALUE(rv_was_called) TYPE abap_bool.

  PRIVATE SECTION.
    DATA mv_exp_number      TYPE i.
    DATA mv_raise_exception TYPE abap_bool.
    DATA mv_result          TYPE i.

    DATA mv_was_called      TYPE abap_bool.
ENDCLASS.



CLASS ltd_calculator IMPLEMENTATION.
  METHOD create_double.
    ro_td_calculator = NEW #( ).
    ro_td_calculator->mv_exp_number = iv_exp_number.
    ro_td_calculator->mv_result     = iv_result.
  ENDMETHOD.


  METHOD create_for_exception.
    ro_td_calculator = NEW #( ).
    ro_td_calculator->mv_exp_number      = iv_exp_number.
    ro_td_calculator->mv_raise_exception = abap_true.
  ENDMETHOD.


  METHOD if_calculator~increase.
    mv_was_called = abap_true.

    cl_abap_unit_assert=>assert_equals( exp = mv_exp_number
                                        act = iv_number ).

    IF mv_raise_exception = abap_true.
      RAISE EXCEPTION NEW cx_sy_arithmetic_overflow( ).
    ELSE.
      " return the prepared calculation result
      rv_result = mv_result.
    ENDIF.
  ENDMETHOD.


  METHOD was_called.
    rv_was_called = mv_was_called.
  ENDMETHOD.
ENDCLASS.

It has two factory methods with which you can set it up for normal use (create_double) or to trigger an exception (create_for_exception). Since the factory methods' return value is TYPE REF TO ltd_calculator, the test class can easily use the test double's helper methods for assertions:

CLASS ltc_any_test IMPLEMENTATION.
  METHOD increase.
    DATA(lo_td_any) = ltd_calculator=>create_double( iv_exp_number = 1
                                                     iv_result     = 2 ).
    " inject calculator double somewhere

    " call method under test which is supposed to use the calculator double 

    " ensure that the method under test called the calculator 
    cl_abap_unit_assert=>assert_true( lo_td_any->was_called( ) ).
  ENDMETHOD.


  METHOD increase_exc.
    DATA(lo_td_any) = ltd_calculator=>create_for_exception( ).
    " inject calculator double somewhere

    " call method under test which is supposed to use the calculator double 

    " ensure that the method under test called the calculator 
    cl_abap_unit_assert=>assert_true( lo_td_any->was_called( ) ).
  ENDMETHOD.
ENDCLASS.

Kind regards,
Jörg-Michael