Design methods and classes in OO

[hagit]

Hello Experts,

Please look at the following scenario:

1. There is an interface & a class, which take care of files

a. ZIF_file_handle

b. ZCL_file_handle

2. In the interface and in the class, there is a method – Import_Excel

3. Import_Excel imports an Excel file to internal table by calling FM ' ALSM_EXCEL_TO_INTERNAL_TABLE'.

4. External programs call this method.

5. Now I have to add a method, which also imports an Excel file. This method will import by using ABAP2XLSX OO. This method has part of the parameters, which Import_Excel has.

My question is how to design the methods?

Do I have to create a new class?

If yes, What is the relationship between the old class and the new one?

Thanks in advance

Hagit

[hagit]

Hello jorgen_lindqvist41 and matthew.billingham

Maybe you can relate to my question?

Thank you in advanced

Hagit

[VXLozano]

Is that last comment a direct ask for help? Do you know how rude it looks?

Do you have to create a new class? Why? Who is in charge of the architecture?

Will the resut of both the old and the new methods the same? (I mean, they will return the same internal table)

If they are, why not just to upgrade the existing one? Just be sure you maintain the contract (the import/export/change/return parameters) the same, and the logical (do nothing the old method does not, and do everything the old method does) and just move that method to this century.

If there is the slightest difference in any point, I'd create a NEW METHOD in the same class (probably in the interface, for cleanliness) explaining in the name what's the difference (ie: import_excel_with_captions).

If there is no difference, but your architect (or anybody) is paranoid about losing things, create a new method too, and name it import_excel_via_abap2xlsx for differentiation.

[hagit]

Some of the external programs will continue to use the old technique and others the new one. Therefor I need both.

[VXLozano]

If you keep the contract (as I told you: import/export/change/result parameters), you don't need to keep the old one.

If you want to keep it, then, as I suggested, create a new one (in the interface) with a meaningful name.

[matt]

I've done something similar. This was my solution.

Create a new implementation of the interface. ZCL_FILE_HANDLER_ABAP2XLSX

Create a factory class. ZCL_FILE_HANDLER_FACTORY with one method, GET_INSTANCE. This method returns an instance of your interface. If abap2xlsx is installed, it returns an instance of your new implementation, otherwise, it returns an instance of your old one.

Clean code recommends that you make your interface implementations CREATE PRIVATE (keeping the constructor public though) and GLOBAL FRIENDS of the factory class. This way, they can only be instantiated through the factory class.

Of course, everywhere we had CREATE OBJECT handler TYPE zcl_file_handler (pre 7.4!), we had to replace it with handler = ZCL_FILE_HANDLER_ABAP2XLSX=>GET_INSTANCE( ).

[matt]

If you need the calling program to specify which implementation to return, you can either create a constant (either in the interface or factory class).

CONSTANTS: BEGIN OF c_excel_handler,
             fm        TYPE string VALUE `fm`,
             abap2xlsx TYPE string VALUE `abap2xlsx`,
           END OF c_excel_handler.

Then the GET_INSTANCE method will have an importing parameter where either c_excel_handler->fm or c_excel_handler->abap2xlsx is passed. The method then returns the desired implementation.

Clean code also recommends the patter of factory class, interface, implementations.

[VXLozano]

I love this guy. He's like an online book about ABAP and OOP.

Let me try to explain to know if I've understood:

- create a class ZCL_FILE_HANDLER_ABAP2XLSX that implements the OP's public interface (ZIF_FILE_HANDLE)

- another class ZCL_FILE_HANDLER_FACTORY with a single GET_INSTANCE method that returns an instance of ZIF_FILE_HANDLE

- if ZCL_EXCEL exists (or something like that), the GET_INSTANCE returns an instance of ZCL_FILE_HANDLER_ABAP2XLSX, and if it does not, an instance of the original class ZCL_FILE_HANDLE

What about other possible non-excel related methods of ZIF_FILE_HANDLE? With the knowledge I'm trying to interiorize, I'd go for a composition: ZCL_FILE_HANDLER_ABAP2XSLX will have an internal object referenced of ZCL_FILE_HANDLE, and every other non-excel method of the interface will call the old referenced one.

Am I right? Or am I, at least, going in the right direction?

[hagit]

matthew.billingham Thank you very much for your answer.

If I create a new implementation of the interface - ZCL_FILE_HANDLER_ABAP2XLSX then

1. Impot by Abap2xlsx needs fewer parameters then the FM. Should I change some of the parameters in interface to optional? Or is there a better solution?

2. In ZCL_FILE_HANDLER_ABAP2XLSX what should I write in the other methods, which are in the interface but do not deal with the import to excel, and so have to be the same as in ZCL_file_handle?

Thank you

Hagit

[VXLozano]

Check my comments under your question (although I'm not Matt). If the contracts are not the same (your new method will require less parameters) then the methods will be different ones. To use optional parameters to allow to use the same implementation is a dirty practice.

[matt]

vicen.lozano It's a reasonable approach. There's not just one right way. I'm still learning techniques.

hagit What you want to get to is that the calling program doesn't need adjusting if you switch from one type of Excel upload to the other. If you don't do that, you've not really abstracted the upload handler; the caller and called programs are tightly coupled, which is a bad thing. Sure, you can create two methods, one for each style, but it's not ideal.

As far as shared code, you can have the abap2xlsx class a subclass of the other one. Though then you need CREATE PROTECTED in the superclass.

I can only present you with general guidelines. It's up to you to figure out how to refactor you design.

[hagit]

matthew.billingham thank you for your answer.

If abap2xlsx class is a subclass of the other one, then it solves the second problem (what should I write in the other methods). But what about the first question (Abap2xlsx needs fewer parameters then the FM)?

You write: " you need CREATE PROTECTED in the superclass." Do mean that I have to declare method 'Import_Excel' as protected?

Thank you.

Hagit

[matt]

First of all, the calling program should not care whether abap2xlsx or the fm is being used. If it does, you've a design problem that needs fixing. IMPORT_EXCEL should just need the name of the file to import and return the data as an internal table (or a reference to a table).

But if you don't want to fix that you need to either make parameters optional or create another method. But if you create another method, then you might as well just add it to ZCL_FILE_HANDLER.

Concerning CREATE PROTECTED. In my initial answer I stated that you should set the implementations of your interface to CREATE PRIVATE, and make the factory class a GLOBAL FRIEND of the implementations. If, however, ZCL_FILE_HANDLER_ABAP2XLSX is a subclass of ZCL_FILE_HANDLER, then ZCL_FILE_HANDLER should be CREATE PROTECTED instead.

[matt]

What is the signature of your IMPORT_EXCEL?

[hagit]

matthew.billingham thank you for your answer.

The signature is

1. The parameters in yellow, which are needed as you wrote

2. The parameters in red, which are needed for the FM . (for Abap2Xlsx I need only im_begin_row)

3. The other parameters return the controls, which were found in the excel file. (Abap2Xlsx deal with the controls in another way so I do not need those parameters)

Thank you.

Hagit

[hagit]

matthew.billingham ,

If I cannot have the same parameters, then what is better? Make parameters optional or create another method in ZCL_FILE_HANDLER (without additional class)

[matt]

Create another method.

[VXLozano]

Definitively (or however it's spelled) the "best" (the easiest of the clean ones) approach is to create another method in the interface, and implement the ZCL_EXCEL calls in the class (obviously).

A dirty one could be to put your ZCL_EXCEL calls into the existing method, ignoring the unused parameters (but keeping them to not break the contract). It's dirty, but it'll work. But it's dirty. Don't. Do. It.

I'm worried about the exporting parameters: are the same as the function modules. So that method is non-sensical. From here it looks just like if someone decided to "go OOP" and just put the FM call into a wrapping method, with no additional logic. What's the point on that?

Matt's approach is by far much better: a new class/method with a non-fixed returning table (data ref, cuz I suppose you cannot use ANY TABLE in a returning parameter, not sure if you can in a CHANGING one, if you can, it may be another easy dirty approach, but much more clean than the first I suggested).

[matt]

vicen.lozano

You can have exporting or changing parameters as tape ANY TABLE. For returning parameters, you can return a reference to the table - this is approach I prefer.

[hagit]

matthew.billingham thank you for your answer.

Would your answer change if you knew in advance that:

1. We need to import excel file to SAP

2. The import is part of file handle

3. There are 2 ways to import

a. Import by FM , which needs some parameters

b. Import by Abap2Xlsx, which needs fewer parameters

vicen.lozano thank you for your remark.

The method does not just wrap the FM, but also finds controls in the Excel and handles it. The exporting parameters are not the same as the FM's parameters. There are additional parameters, which handle the controls.

[VXLozano]

Then add a new method to the interface, and implement it in the existing class. Do not use optional parameters if you really don't need them (optional parameters usually are signs of wrong design). Your main problem will be the returning parameter: as Matt pointed, probably you will return a reference to data. I don't like them (because I don't manage them often). If the CHANGING clause supports ANY TABLE parameters, I'd go that way (because I'm lazy).

(edited: just read Matt's comment before yours... CHANGING ANY TABLE it is, then)

[hagit]

vicen.lozano Thank you for your help.