Skip to Content
Technical Articles
Author's profile photo Michael Keller

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.

Assigned tags

      6 Comments
      You must be Logged on to comment or reply to a post.
      Author's profile photo Jörgen Lindqvist
      Jörgen Lindqvist

      Yeah, that won't work, thanks for the warning and the reminder to use singletons mindfully... 🙂 Luckily, you have recently also written a blog post about the multiton in Santa Claus and the christmas elves, as another way of getting this code to work as the developer assumed.

      Author's profile photo Michael Keller
      Michael Keller
      Blog Post Author

      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.

      Author's profile photo Michael Biber
      Michael Biber

      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".

      Author's profile photo Suhas Saha
      Suhas Saha

      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 🤷‍♂️

      Author's profile photo Jacques Nomssi Nzali
      Jacques Nomssi Nzali

      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

       

      Author's profile photo Enno Wulff
      Enno Wulff

      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...?