Object oriented report design

[Former Member]

Hi together,

I have developed a report and I tried to do it in a object oriented way. I'm still in a  "transition phase" coming from procedural ABAP to object-oriented ABAP. The report is doing exactly what it should do, so I need no help in functionality.

What I like to discuss is the design of the report or better how can I improve the design?

In general I've tried to design the report based on the MVC design pattern. So I have created a class for the Controller, the Model and the View. Additionally I have creted a own class for the Selection-Screen, which has the parameters as attributes and the report events as own methods.

The controller has one main method MAIN, which controls the program flow.

The Model has two main methods, GET_DATA and POST_DATA. Based on a Parameter from the selection-screen the GET_DATA should read the data in a different way. The POST_DATA method is always calling a BAPI in the same way. So GET_DATA always should provide the data in the same way, that POST_DATA can use it. First I wanted to implement Strategy design pattern for the GET_DATA method, but I decided to use the new Filter-dependent BAdi (which is like a tool supported version of the Strategy). So the GET_DATA method calls a Badi method GET_DATA which can be implemented different, based on the parameter from the selection screen.

The generated UML class diagram looks like this.

The Model class and Selection-Screen class are attributes within the Controller class. The Selection-Screen class is also an attribute in the Model class. I'm not sure here. Wouldn't it makes sense to have the selection screen static, which means you could get the attributes from everywhere? Wouldn't it make it easier or are there any advantages to have it as an object reference included?

The main report looks like this.

*&---------------------------------------------------------------------*
*& Report  ZCO_ZR22
*&
*&---------------------------------------------------------------------*
include zco_zr22_top.
 *--- Global Objects ---------------------------------------------------*
data:
  go_sel_screen type ref to zcl_co_zr22_sel_screen,
  go_controller type ref to zcl_co_zr22_controller.

*&---------------------------------------------------------------------*
load-of-program.
*&---------------------------------------------------------------------* 
* Create Selection Screen object
  create object go_sel_screen
    exporting
      im_report_id = sy-repid.

*&---------------------------------------------------------------------*
initialization.
*&---------------------------------------------------------------------*
  go_sel_screen->initialization(
    changing
      ch_budat = p_budat
      ch_poper = p_poper
      ch_bdatj = p_bdatj ).
*&---------------------------------------------------------------------*
at selection-screen on value-request for p_kongr.
  
go_sel_screen->on_value_request_for_kongr(
    changing
      cd_kongr = p_kongr ).
*&---------------------------------------------------------------------*
at selection-screen.
*&---------------------------------------------------------------------*
    go_sel_screen->at_selection_screen( ).
*&---------------------------------------------------------------------*
start-of-selection.
*&---------------------------------------------------------------------*
* Create Controller object
  create object go_controller
    exporting
      im_sel_screen = go_sel_screen.
* Start processing
  go_controller->main( ).

I'm really not sure if this is the right way to instantiate the objects, but I do not see an alternative, especially for the Selection-Screen object.

The Controller class has an Constructor where I create the Model class, Message Handler and set the Selection Screen Object. The Selection-screen object is also assigned to the Model after creation. Is this correct?

I have created the Message Handler as static attribute, so I could use it also from within the Model object, but I'm not sure this is correct. Is it common to mix static and instance attributes/methods within a class? Does it have any disadvanteges to do it like this?

* <SIGNATURE>---------------------------------------------------------------------------------------+
* | Instance Public Method ZCL_CO_ZR22_CONTROLLER->CONSTRUCTOR
* +-------------------------------------------------------------------------------------------------+
* | [--->] IM_SEL_SCREEN                  TYPE REF TO ZCL_CO_ZR22_SEL_SCREEN
* +--------------------------------------------------------------------------------------</SIGNATURE>
method constructor.

* Create Message Handler
  mo_msg_handler = cf_reca_message_list=>create(
    id_object    = 'ZCO_PC'
    id_subobject = 'ZCO_ZR22' ).

* Set selection screen attributes
  mo_sel_screen = im_sel_screen.

* Create Model Object
  create object mo_model.

* Pass selection screen attributes to model
  mo_model->mo_sel_screen = mo_sel_screen.

endmethod.

Within the MAIN method of the controller class I call first the GET_DATA than POST_DATA. The ALV is than generating the output.

* <SIGNATURE>---------------------------------------------------------------------------------------+
* | Instance Public Method ZCL_CO_ZR22_CONTROLLER->MAIN
* +-------------------------------------------------------------------------------------------------+
* +--------------------------------------------------------------------------------------</SIGNATURE>
method main.

  data:
    lo_exc_get_data type ref to zcx_co_get_data,
    lo_salv_table   type ref to cl_salv_table,
    lo_salv_msg     type ref to cx_salv_msg,
    lo_salv_events  type ref to cl_salv_events,
    lo_zr22_view    type ref to zcl_co_zr22_view.

  data:
     lf_msgv1 type sy-msgv1.

* --- Start processing ------------------------------------------------*
  if mo_sel_screen->md_rev = abap_true.
    lf_msgv1 = text-003.
  endif.

  if mo_sel_screen->md_test = abap_true.
    lf_msgv1 = text-001.
  else.
    lf_msgv1 = text-002.
  endif.

  mo_msg_handler->add(
    exporting
      id_msgid = 'ZCO_PC'
      id_msgty = 'I'
      id_msgno = '001'
      id_msgv1 = lf_msgv1 ).

* --- Get data from model ---------------------------------------------*
    try.
        mo_model->get_data( ).

      catch zcx_co_get_data into lo_exc_get_data.
        mo_msg_handler->add_from_exception(
          exporting
            io_exception = lo_exc_get_data ).

    endtry.

* --- Post data with MR22 ---------------------------------------------*
  if mo_sel_screen->md_test = abap_false.

    mo_model->post_data( ).

  endif.

* --- Output ALV and Application Log ----------------------------------*
  if mo_sel_screen->md_test = abap_true.

    build_list( ).

    try.
        cl_salv_table=>factory(
          importing
            r_salv_table = lo_salv_table
          changing
            t_table      = mt_list_testrun ).

      catch cx_salv_msg into lo_salv_msg.
*     Message zur Beendigung bei Fehler und Ausgabe am Screen
        message lo_salv_msg type 'I' display like 'E'.
        exit.
    endtry.
  else.

    try.
        cl_salv_table=>factory(
          importing
            r_salv_table = lo_salv_table
          changing
            t_table      = mo_model->mt_mr22_post ).

      catch cx_salv_msg into lo_salv_msg.
*     Message zur Beendigung bei Fehler und Ausgabe am Screen
        message lo_salv_msg type 'I' display like 'E'.
        exit.
    endtry.

  endif.

  lo_salv_table->set_screen_status(
    pfstatus      = 'SALV_TABLE_STANDARD'
    report        = mo_sel_screen->md_report_id
    set_functions = lo_salv_table->c_functions_all ).

  create object lo_zr22_view.

  lo_zr22_view->process_functions(
    exporting
      io_salv_table = lo_salv_table ).

  lo_zr22_view->process_layouts(
    exporting
      io_salv_table = lo_salv_table
      id_layout     = ' '
      id_report_id  = mo_sel_screen->md_report_id ).

  lo_salv_events = lo_salv_table->get_event( ).

  set handler on_user_command for lo_salv_events.

  lo_salv_table->display( ).

endmethod. 

I'm not very happy with the "View" part of my design. I had the problem that if it is a Testrun, I have to create the a different ouput list, so I didn't know how to pass two different tables to the same method. How could this part improved? At the moment I have the feeling that it is not the right way to do it, from a design point of view.

I would like to hear your opinion or what can be improved or not. Any suggestion or comment is appreciated.

Thanks & regards,

Tapio

[nomssi]

Hello Tapio,

to me, good design is code that seems to solve your problem nearly effortlessly. I tried to understand your code in a few minutes and I could not feel a sense of understanding. The report seems to hide its purpose. Instead, it felt like decrypting another bit of undocumented code.

Your model class ZCL_CO_ZR22_MODEL (naming?) has GET_DATA( ) and POST_DATA( ). So.. it is some proxy for a database table? you want to post entries? create new entries or update existing?

Let me rename it ZCL_DATA and your method get( ) and post( ). get_posted_data( ) would be get_persistent( ) in my book. and from your talk about strategy, get( ) would be a client of a service class. By now, my mental model of your application is:

selection screen  => parameters

my client get data

my client post data.

(I nearly stopped at this level)

hmm, where is the editing? OK, you do not need any. Are you reading data files and saving those information in a database? well, this is what is seems to me. And after saving, you collect the logs from the BAPI and create a neat ALV.

So your code is following the Input, Processing, Output pattern, which is fine, until you try MVC, but do not switch to a multi-layered architecture. Well, I do not have to understand your design, but you should be able to discuss it qualities and shortcomings with your co-worker. To improve it, I suggest 1 or 2.

1. the work hard way: refactor mercilessly. Start with the code smells, the pieces where you are at unease with the code and clean the mess up, once piece at a time. With progress, you will have a better uncovered view of the current design and will be in a better position to let it evolve toward a more supple design.

2. the think hard way: how would you describe the problem you are trying to solve in one phrase? which words would you use? are this words part of your object model? You have created a solution for a given problem: explain the problem, the constraints, and you point of view. Understand strength and limitations of your design. This is what you should be discussing in the first place

(are you still with me?)

P.S.:

1. if you don't really need a static attribute, do not create one

2. you can pass 2 different table to the same method by declaring a generic table as parameter,

e.g. METHODS view_table CHANGING ct_table TYPE STANDARD TABLE.

3. I suggest moving the user-event handler stuff from the SELECTION class into the Controller, renaming what is left into a PARAMETER Class to reduce dependencies.

my opinion.

JNN

[Former Member]

Hi Jacques,

thanks for taking time and replying to my post. I really appriciate your thoughts.

to me, good design is code that seems to solve your problem nearly effortlessly. I tried to understand your code in a few minutes and I could not feel a sense of understanding. The report seems to hide its purpose. Instead, it felt like decrypting another bit of undocumented code.

Hmmm, was it really that difficult to understand? I left all the details out by purpose, because I think it has no effect of the general design of the report.

Your model class ZCL_CO_ZR22_MODEL (naming?) has GET_DATA( ) and POST_DATA( ). So.. it is some proxy for a database table? you want to post entries? create new entries or update existing?

The class name has some naming conventions from client, but could also be called ZCL_MODEL. From a design point of view I don't think it makes a big difference to call it ZCL_MODEL or ZCL_DATA.

Let me rename it ZCL_DATA and your method get( ) and post( ). get_posted_data( ) would be get_persistent( ) in my book. and from your talk about strategy, get( ) would be a client of a service class.

Regarding the naming of the methods I tried to follow the rules SAP is providing. See

http://help.sap.com/abapdocu_731/en/abentelling_names_guidl.htm

What exactly do you mean with "client of a service class"? The controller class or a separate class?

By now, my mental model of your application is:
selection screen  => parameters
my client get data
my client post data. 
(I nearly stopped at this level)

In fact, that's almost all. I want to read data from different sources and post it always through a standard BAPI in the same way. Ok, I write also some data in database tables as a control record and the posted data by the BAPI, so if I have to reverse the run I just can read the posted data. But has this really an influence on the overall design? It's just decided in the GET_DATA method, based on the parameters on the the selection screen. The controller shouldn't take care from the data comes, whether it's read from an excel file or coming as reverse postings from some log tables. Do you understand?

hmm, where is the editing? OK, you do not need any. Are you reading data files and saving those information in a database? well, this is what is seems to me. And after saving, you collect the logs from the BAPI and create a neat ALV.

Yes there is some transformation of the source data into the format the BAPI can handle it. This is enabled as a private method in model class, because I thought it is not of interest for the controller (or client?) how this is done.

So your code is following the Input, Processing, Output pattern, which is fine, until you try MVC, but do not switch to a multi-layered architecture. 

What do you mean with multi-layered architecture in this example?

Well, I do not have to understand your design, but you should be able to discuss it qualities and shortcomings with your co-worker. To improve it, I suggest 1 or 2.

Yes agree, but this is one of my biggest problem. Most of my co-workers are developing not in a object-oriented way. That's why I try to get some opinions from here.

1. the work hard way: refactor mercilessly. Start with the code smells, the pieces where you are at unease with the code and clean the mess up, once piece at a time. With progress, you will have a better uncovered view of the current design and will be in a better position to let it evolve toward a more supple design.
2. the think hard way: how would you describe the problem you are trying to solve in one phrase? which words would you use? are this words part of your object model? You have created a solution for a given problem: explain the problem, the constraints, and you point of view. Understand strength and limitations of your design. This is what you should be discussing in the first place
(are you still with me?)

The report should get data from different data sources and post it through a standard BAPI into the system. The posted data should be displayed in an ALV list and a log can be called to see all messages the report generates. Thats all...

1. if you don't really need a static attribute, do not create one

Why?

2. you can pass 2 different table to the same method by declaring a generic table as parameter,
e.g. METHODS view_table CHANGING ct_table TYPE STANDARD TABLE.

Yes, I will check this possibility. But is it correct to include the display method in the view class or should it be in the controller class?

3. I suggest moving the user-event handler stuff from the SELECTION class into the Controller, renaming what is left into a PARAMETER Class to reduce dependencies.

The Event handling for the ALV is in the controller class. I thought the report events which are related to the selection screen parameters should be part of the selection screen class.

my opinion.

Thanks a lot for it!

Best regards,

Tapio

[nomssi]

 I left all the details out by purpose, because I think it has no effect of the general design of the report. 

are you delivering a solution for the right problem? Without requirements, I can only discuss clean code, not good design.

What exactly do you mean with "client of a service class"? 

Objects are powerfull when they encapsulate behavior in class with  both data (state) and methods (functions).A class without method is a "dumb" data class. I call a class without attributes a service class. They are used i.e. in the strategy pattern.

Ok, I write also some data in database tables as a control record and the posted data by the BAPI, so if I have to reverse the run I just can read the posted data. But has this really an influence on the overall design?

Yes. This undo ability as an important requirement. it makes me understand why you bother to save.

What do you mean with multi-layered architecture in this example?

IPO means you decompose your problem in function/operations. Layered Architecture means partitioning a program into layers, e.g. separating the model from the infrastructure, allowing a cleaner design of each layer (cf. DDD). MVC is an example of layered architecture.

1. if you don't really need a static attribute, do not create one
Why?

you are giving up some power.

Some design discussion now:

You have a SELECTION_SCREEN, MODEL, VIEW, CONTROLLER

MODEL Class might have

• Convert( )  - convert into a given BAPI format (private?)
• Post( ) – create DB entries using BAPI
• Reverse( ) – undo some postings

for the requirement you stated, this could possibly work.

What I suggested to achieve the decoupling was a dumb PARAM data class that is created by the SELECTION and passed to the MODEL.

The MODEL should not be linked the SELECTION (User Interface Layer). The CONTROLLER should link both.

A READER interface/abstract class can encapsulate the data fetching method with the methods

• Factory( ) - takes a PARAM an creates a concreate READER object
• Get_data( ) - abstract data fetching method.

You will then need some concrete e.g. FILE_READER/EXCEL_READER service classes as needed.

SELECTION class might have

• Get_Selection( ) - reads the selection parameter and returns a PARAM object

VIEW

• display( )

CONTROLLER class might have

• attributes SELECTION, MODEL, READER, PARAM, VIEW
• Selection( ) - Creates PARAM using SELECTION
• Get_data( ) - read input from a data source – delegates to READER class
• Process( ) - call MODEL methods Convert( ) and Post( ) or Reverse( ) depending on the Parameter. Call VIEW display( ) method.

The modeling exercise for me was to name your classes and assign responsabilities to then. This is a critical point of OO design. Take a few minutes for this before programming. After programming, document your current classes and responsabilities.

I use these documents (be it UML or any kind of text description like this one) to discuss with my co-workers. If I cannot convince them the set of classes is a solves a given problem then, well, it is probably not a good model.

We Are All Learners.

JNN

[Former Member]

Hi Tapio,

It is really nice to experiment with OO Designs in the traditional ABAP framework. I am also an ABAP developer using OO patterns for the past 2 years or so , so based on my little experience here are my observations  I believe Jacques Nomssi has given you a clear detailed explanation, but here are my thoughts.

Before i go into my thoughts, thanks to you it has stirred a few thoughts on OO design for reports. Previously i used to use the SAP Report driver in itself as a screen controller, but looking at your design, it has made me think otherwise. I am pondering what would be the actual benefits of using another layer of screen controller.

1. I could see there are 2 controllers used, the Main controller(ZCL_CO_ZR22_CONTROLLER) and then the selection screen controller.

     a. The user command should be handled in the Selection screen controller. The control should then pass to the Main controller and then on to the Model

     b. The Selection screen controller should have a reference to the Main controller.

     c. The model should never contain a reference to any of the controllers.

    d. The Main controller - main method  is not responsible for the display. If you have  a specific class for handling the display of data , in this case the View class, the display and setting of layouts should happen only there. This view class should in turn control the screen display.

To answer your question on how to make the View class better, pass the table data as a generic data type, use RTTS if needed inside the view class. The main method is a clear violation of Seggregation of Duties.

Also, the View controller should be an attribute of the Main controller, as there maybe some interactive events in the ALV in the future that needs to be responded by the Model.

   e. Why is there no exception handling for POST method, there is a possibility that the BAPI might return some erroneous data. All exceptions handlings are to be consistent , especially in the case of interface(public) methods.

f. Implement the factory pattern especially for the model and the main controller. I am skeptical about the selection screen as there could be more than one (sub) selection screens in a selection screen.

g. If you want to Implement MVC for a report, you have to define the selection screens as sub screens, i would assume you would have done it. So that the whole idea of the component ( screen + model ) could you be plugged in any dynpro.

Thanks,

Venkat.

[Former Member]

Oops I forgot to add my Disclaimer.

If i was wrong in any of the above points, please feel free to challenge me. OO design in itself is Artistry and there is no perfect 'artist'. We all have our own visions and insights.

Another point i would like to add, Make sure that the Model has another layer on top sort of a model controller which would have only Public Methods. The reason being that this give you the flexibility to choose a model dynamically.

Once again thanks for the Brain stirring post.

Thanks,
Venkat.

[Former Member]

Hi Venkat,

I have now a little bit time to reply to your comment. Thanks you for your input.

1. I could see there are 2 controllers used, the Main controller(ZCL_CO_ZR22_CONTROLLER) and then the selection screen controller. 
     a. The user command should be handled in the Selection screen controller. The control should then pass to the Main controller and then on to the Model
     b. The Selection screen controller should have a reference to the Main controller. 
     c. The model should never contain a reference to any of the controllers. 
    d. The Main controller - main method  is not responsible for the display. If you have  a specific class for handling the display of data , in this case the View class, the display and setting of layouts should happen only there. This view class should in turn control the screen display.

I'm not sure the Selection-Screen class is another controller. It is just a class which contains all the standard events for selection screen processing and keeps all the parmeters and select-options.

   e. Why is there no exception handling for POST method, there is a possibility that the BAPI might return some erroneous data. All exceptions handlings are to be consistent , especially in the case of interface(public) methods.

Yes, there is error handling, but it is directly after the Bapi call. But this leads to another question. Where is the appropriate place for the error handling?

Should it be propagated to the controller level or should it be at the individuell layer. But than the Message-Handler class must be passed to each layer or make it global available?

g. If you want to Implement MVC for a report, you have to define the selection screens as sub screens, i would assume you would have done it. So that the whole idea of the component ( screen + model ) could you be plugged in any dynpro.

I have read so many about MVC pattern the last view weeks and I ahve to say I'm quite confused. In some descriptions every view should have it's own controller and also the Observer-Pattern should be apllied if model interacts with view. I'm not sure this is too much for a report like this. There is no interaction with the selection-screen.

Now I need to re-design the report.

Tapio

[Former Member]

Hi Tapio,

Thanks for your reply.

To answer your queries:

I'm not sure the Selection-Screen class is another controller. It is just a class which contains all the standard events for selection screen processing and keeps all the parmeters and select-options.

It is a controller .In fact i would put it down as a screen(view) controller. What are the typical responsibilities of a view controller -

1. Data handling in the UI. 

2. Event handling in the UI.

3. Data validations in the UI ( not back end logic validations). and some more...

I would think that the selection screen class already does the above mentioned. Infact it is very good that you have designed a screen controller .

One more point, you have designed the Selection screen object keeping in mind only one selection screen, you might have to change it a bit as there might be sub screens and you would have to get one  selection screen object for every sub screen, so that each of the screen object has its own validations, F4 helps and so on. 

Yes, there is error handling, but it is directly after the Bapi call. But this leads to another question. Where is the appropriate place for the error handling? 
Should it be propagated to the controller level or should it be at the individuell layer. But than the Message-Handler class must be passed to each layer or make it global available?

Well, I cant debate much as I am not an expert on this, but the reason why i raised the issue was that you have exception handling for the get method but nothing for the post method. The preferred Idea would be to have the same for all methods .

In the case of a message handler, I am not much aware of its uses, typically I use message handler in BAL. If you want any message to be displayed in the UI, then propogation would be good as it would be propogated all the way to the UI controller. If you want to maintain a log of messages, then the message handler would be a better option.

In the case of message handler , the typical implmentation would be to use it as a service/assistance class to all classes. It is preferable not to maintain them  as a static attribute in any of the classes .

The message handler class should have only static methods such as - >add_message(), get_messages(), remove_message_from_list(). I understand you use the standard SAP class, but you could always wrap it in a Z message handler class and use static methods on top.

Also i noticed that the Main controller calls the get_data() and Post()- well you should not call these methods technically. These are either protected or private methods of the model class. Why should the controller be aware of the methods the model has?. I mean if you view the model as a service, then there should be only one public method - > POST_DATA_TO_XXX() - called from the main controller which takes in the input and throws an exception as needed/success return messages. This method would internally call the Get_data() and post_data()- this is encapsulation.

See the model as a service, then you would realise that only a few methods should be public.

I have read so many about MVC pattern the last view weeks and I ahve to say I'm quite confused. In some descriptions every view should have it's own controller and also the Observer-Pattern should be apllied if model interacts with view. I'm not sure this is too much for a report like this. There is no interaction with the selection-screen.

Ok - MVC to keep it in simple words promotes re usability through component development. As programming languages evovled, once such evolution of Object Orientation is component based development. The whole idea of a component is to bundle both the screen/views and the logic as a single re usable unit. If you think of that way, then the only option in SAP GUI would be to use the views as a Sub screen or as a popup. Imagine this , tomorrow there is a  new requirement, where they need 4 functionalities in one screen and one of the functionalities is this report . If you had designed this as a subscreen or a popup, then all you need is just a screen area/button to plug in/call  your dynpro.

Regarding the observer pattern, it is used in scenario's of distributed event handling. Let say, a user set adjusts his personal settings in an application - say Time zone. In that case, the observer pattern ensures that the date and time attributes in the dependent objects are automatically adjusted for the the new time zone-How you do it is your choice - Raise events or maintain a list of objects.  . In the MVC paradigm- Any changes in the component controller, should automatically be propogated to all the screen controllers, this could be one such implementation of the Observer pattern.

Now I need to re-design the report.

Thats why i had mentioned , its always an experiment to adopt OO design in the traditional framework. If you were to incorporate all these changes, the development time would exponentially rise , but you would end up with a big list of re usable objects that could speed up future developments.

Keep in mind, when you design the selection screen class, try to keep it generic so that you could use it in future reports you would build.

Again , I would emphasize i have nothing against your design, I am just sharing my perception.

I hope this helps.

Thanks,

Venkat.