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.
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.
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.
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
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...?
Hey Michael Keller
I got your blog suggested next to my blog about "Multiton in WM/EWM"
and I write this comment right after seeing the first lines of your code (I didn't peek further down, honestly!) .
I'm pretty sure the problem you address is that the BUKRS member makes it not suitable for singleton, it should be a multiton. Like LGNUM in my case. Right? 🙂
Best
Joachim
Not exactly and yet somehow 😉 I think you get the point across: there are organizational elements that are unique in a client, but by their very nature are not alone.
My blog was also a bit about time. At some point a Singleton is really ok. But the world around it changes and then it has to be a Multiton. Only nobody thinks of such a thing 😉