Skip to Content
Technical Articles

A dangerous assumption

Dear community, the Clean ABAP styleguide recommends to carefully check if a Singleton design pattern should be applied. Some time ago I’ve seen a use case of Singleton with a dangerous assumption that finally led to an error. I’ve set up a similar example for discussion. Here it is…

Our class ZCL_ANY_CLASS uses the Singleton design pattern for some unknown reason 🙂

CLASS zcl_any_class DEFINITION
                    PUBLIC
                    FINAL
                    CREATE PRIVATE.

  PUBLIC SECTION.
    DATA mv_company_code TYPE bukrs READ-ONLY.

    CLASS-METHODS get_instance
      IMPORTING
        iv_company_code  TYPE bukrs
      RETURNING
        VALUE(ro_result) TYPE REF TO zcl_any_class.

    METHODS constructor
      IMPORTING
        iv_company_code TYPE bukrs.

  PRIVATE SECTION.
    CLASS-DATA mo_instance TYPE REF TO zcl_any_class.

ENDCLASS.


CLASS zcl_any_class IMPLEMENTATION.

  METHOD get_instance.
    IF mo_instance IS NOT BOUND.
      mo_instance = NEW zcl_any_class( iv_company_code ).
    ENDIF.

    ro_result = mo_instance.
  ENDMETHOD.

  METHOD constructor.
    mv_company_code = iv_company_code.
  ENDMETHOD.

ENDCLASS.

The class is used as follows. Our “virtual” client in our demo scenario currently only has one active company code. So far all right.

DATA lo_any_class TYPE REF TO zcl_any_class.

lo_any_class = zcl_any_class=>get_instance( '1000' ).
cl_demo_output=>write_data( lo_any_class->mv_company_code ).

cl_demo_output=>display( ).

Here’s our demo output.

To complicate things now (as life is like that): Later this class is used by another developer in another application. There are now several active company codes in our “virtual” client. The dangerous assumption: what works for one company code is sure to work for several.

TYPES: BEGIN OF ty_company_code,
         company_code TYPE bukrs,
       END OF ty_company_code.

TYPES ty_company_codes TYPE TABLE OF ty_company_code WITH KEY company_code.

DATA lo_any_class TYPE REF TO zcl_any_class.

DATA(lt_company_codes) = VALUE ty_company_codes( ( company_code = '1000' )
                                                 ( company_code = '2000' ) ).

LOOP AT lt_company_codes INTO DATA(ls_company_code).
  CLEAR lo_any_class.
  cl_demo_output=>write_data( ls_company_code-company_code ).
  lo_any_class = zcl_any_class=>get_instance( ls_company_code-company_code ).
  cl_demo_output=>write_data( lo_any_class->mv_company_code ).
ENDLOOP.

cl_demo_output=>display( ).

Here’s our demo output.

Ouch! The first call of GET_INSTANCE binds our instance to company code “1000”. The second call for company code “2000” only returns the instance for company code “1000”.

A separate method “SET_COMPANY_CODE” could be used here to solve our problem – but that’s maybe a poor solution according to the design. Another solution could be the use of the Multiton (see the comment of Jörgen Lindqvist). In short: The original design with the IMPORTING parameter via GET_INSTANCE/CONSTRUCTOR no longer fits.

What is still quite manageable in this example can be difficult to recognize in practice. I guess that in an implementation of the INITIALIZE method of the BAdI “ME_PROCESS_PO_CUST” something like this can also be problematic. As long as purchase orders are only worked for one company code, that’s okay. As soon as you open the purchase order from another company code in the same working session, you have a problem because of the Singleton.

 

Best regards, thanks for reading and stay healthy

Michael

 

P. S.: Not tired of reading? Check this blog by Cristoph Pohl.

6 Comments
You must be Logged on to comment or reply to a post.
    • Jörgen, thanks for the hint. It is great what you contribute here! 🙂

      Multiton is the much better solution here. I have therefore adapted the blog text.

  • I would even go further: never code against a current situation if the SAP Standard can handle more.

    Had something similar recently: "we always have only one concurrent interest condition".

  • Generally i let a factory decide whether to return a singleton or create a fresh instance. You should anyway not program to a class, rather an interface.

    "! API to access company customizing
    interface LIF_COMPANY.
    endinterface.
    
    "! Concrete company implementation
    class LCL_COMPANY definition create private friends LCF_COMPANY.
      public section.
        interfaces LIF_COMPANY.
        methods CONSTRUCTOR
          importing
            I_COMPANY_CODE type T001-BUKRS.
      private section.
        data _COMPANY_CODE type T001-BUKRS.
    endclass.
    
    class LCL_COMPANY implementation.
      method CONSTRUCTOR.
        _COMPANY_CODE = I_COMPANY_CODE.
      endmethod.
    endclass.
    
    "! Factory of companies
    class LCF_COMPANY definition abstract create private.
    
      public section.
        "! Gets the company singleton
        class-methods GET_COMPANY
          importing
            I_COMPANY_CODE  type T001-BUKRS
          returning
            value(R_RESULT) type ref to LIF_COMPANY.
    
        "! Creates a company instance
        class-methods CREATE_COMPANY
          importing
            I_COMPANY_CODE  type T001-BUKRS
          returning
            value(R_RESULT) type ref to LIF_COMPANY.
    
      private section.
        class-data _COMPANY_AGENT type ref to LCL_COMPANY.
    
    endclass.
    
    class LCF_COMPANY implementation.
      method GET_COMPANY.
        if _COMPANY_AGENT is not bound.
          _COMPANY_AGENT = new #( I_COMPANY_CODE ).
        endif.
        R_RESULT = _COMPANY_AGENT.
      endmethod.
    
      method CREATE_COMPANY.
        R_RESULT = new LCL_COMPANY( I_COMPANY_CODE ).
      endmethod.
    endclass.

     

    PS: I am not saying to have the GET_COMPANY( ) & CREATE_COMPANY( ) in the same factory. I can't think of a valid reason why one would even want to do that 🤷‍♂️

  • As I see it, an undocumented design decision leads to a ... surprise.

    Throwing an exception in GET_INSTANCE( ) if the global variable (hmm.. singleton) is bound to another value (company code) is a simple change that would explicitly document the behavior for the client.

    JNN

     

  • The singleton pattern is not coded properly. The general query IF object IS NOT BOUND can only be used if there are no importing parameters within the CONSTRUCTOR. In your case you should have used

    IF object IS NOT BOUND
    OR object->mv_company_code <> iv_company_code.

    It's not necessary to built a multiton of that just because the singleton is doing wrong.

    Of course I agree: you should rely on assumptions like the code is doing what I expect it to do.

    This behavior is even more evil as it might not work for only just ONE company code but maybe for 10 or more. Until you get a company code that uses a different currency than EURO... 😁 In all other cases it might not even be noticed for a long time that all the same object is being used.

    Afterwards you might say: It would not have happened if you wrote unit tests for this.

    On the other hand you will not write unit tests for "simple" and "basic" functionalities.

    A good reminder that the greatest flaws might be caused by the simpliest functions...?