Refactoring an ABAP Include Monolith

[nothafts]

Motivation

Usually, in R/3 you see a common pattern: Spaghetti-Code within of program includes of a certain function group. Especially, in case of extensions of SAP standard this was required and caused the creation of big monoliths, whereby more than 10.000 lines of code was not rarely.

In addition, another problem as consequence is that only one developer could do a change of the source code, because the whole function group was locked. I would also assume that those changes could not be tested very well. Therefore, the developer would not be aware of any side effects.

This document tries to help you solving that problem.

Objectives

I would like to define the following objectives:

• Code has to be changeable


• The developers have to be enabled to implement requirements w/o any risk of side effects to existing functionality and also to work in parallel


• Not all user have to be affected by the refactoring è reduce risk of complete failure

Non-Objectives:

• The creation of micro services


• Leverage (S/4)HANA features


• Only refactor code, which is really being used (see SCMON/simplification process)

Context

• Users (also machines, sensors) use the coding in production. We have to be careful!


• Probably, the quality of the source code is very low. I would expect there:
  
  
  
  • Bad identifiers for variables/constants
  
  
  • Commented coding
  
  
  • Literals
  
  
  • Spaghetti-Code / redundant coding
  
  
  • Call of subroutines (PERFORM)
  
  
  • Use of “dirty-assign” to other function groups
  
  
  • Direct database accesses
  
  
  
  

Procedure

Apparently, step „zero“ is to get the permission and the budget/time for such a big refactoring.

Also mission-critical is the moment to begin with the refactoring: I would recommend starting before the conversion to S/4HANA, because the conversion could rise syntax errors or other incompatibilities. Unfortunately, not every feature of ABAP unit will be supported on lower NetWeaver releases. However, if you could transfer the old coding w/o any problems to S/4HANA, you can start there as well.

Generally, I recommend as IDE the ABAP Development Tools (ADT) to perform the refactoring, as it is superior to classic SAP GUI.

1. Freeze coding: Only very urgent hotfixes are allowed. Further development is prohibited


2. Create a backup: You should create a snapshot of the current coding (e.g. via git). With that you have a “plan B” and maybe can go back, if something goes completely wrong


3. Do simple refactoring: Rename variables, introduce constants, delete commented code à should be possible w/o any risk


4. Prepare test data:
   
   
   
   1. Create a testgroup with appropriate test user: In the best case, you have a wide diversity and involve several departments of the company to achieve a good test data quality
   
   
   2. Create a clone of the include in an ABAP OO class (let’s call it simply “agent”)
      
      
      
      1. Provide a simple entry method, like “execute” to call it from the original function group
      
      
      2. Encapsulate all dependent data of the original function group. I would recommend to create a dedicated class for that (name: “Origin Function Group API”)
      
      
      3. Calls to local subroutines also have to be extracted to the Origin Function Group API. Reason: Probably they would operate on the data of the function group
      
      
      
      
   
   
   3. Encapsulate external accesses in the agent: Calls of the same function modules (incl. dirty assigns) and access to database tables should be refactored into appropriate classes. Let us call them “DB Access”, respectively “Other Function Modules API”.
      In order to avoid a direct coupling you should work with ABAP OO interface. You could also create the instances of the classes by using a “Dependency Lookup”
   
   
   4. Integrate a “Test Spy” into the classes: The spy is an additional class, which logs the input- and output parameter of the method calls. I would recommend to use one Spy for all classes, because you can also log the sequence of method calls. The log data should be stored into particular database tables or in an eCATT-test data container; I prefer the eCATT-container.
   
   
   5. Integrate agent into origin function group with a user parameter:
      
      
      
      1. Once the users of the test group have activated their user parameter, they should do their usual business
      
      
      2. In addition I also recommend to activate the code coverage analyzer for the test group in order to find also some blind spots
      
      
      3. Dependent of the program include, you should record the test data appropriately. For example, if the coding is only relevant at year end closing, you should consider this time period in your recording
      
      
      4. Until now, some coding already has been changed, so there could be a small risk, that you have made some mistakes. Therefore, the test group should also keep an eye on the functional correctness. Of course, in case of any errors, they have to be fixed immediately.
      
      
      
      
   
   
   
   

Intermediate result:

• There is no chaotic read to the function group’s data. Instead you use an API


• Also other accesses (database tables, other function modules) are (well) defined now


• You are able to create test data for the automatic testing. This is the fundament for a more complex refactoring, which we want to perform in the next steps

Class diagram (high level):



Hint: During the recording, you as a developer could already think about, how to split this big agent class into appropriate business function classes. Surely, you could start with the first test data a prototype in a local copy of the agent. (see also the next steps)

1. Provide unit test infrastructure for the agent class: Dependencies have to be „mocked“ and provided with the data from the test spy (expected input/output data, calling sequence)


2. The unit test should be successfully at the first time: If not, maybe you need to adjust the unit test infrastructure or one of the other classes


3. Start the complex refactoring: Extract method/classes (see also „Separation of Concerns“-principle). You should also use ABAP OO interfaces to decouple from the concrete classes. Recommendation: Start first with extracting some method. By the name of the methods, you could see if there are common aspects (e.g. country specific) and extract them to new classes, so called business functions


4. Extract parts of the “global” test data from the spy to the business function: At the agent, all test data for all business functions are maintained, but this raises also the complexity. Moreover, you would not be able to implement new requirements in parallel. Therefore, you have to extract the relevant test data also the business functions unit test. Usually, the redundant test data is obsolete and you can remove them from the eCATT test data container.


5. The agent only orchestrates calls to the business functions: The unit test infrastructure should only test the agent, not the business functions itself


6. Adapt test spy, if required. I recommend, to create after step 5 to create a new test data container, as a preparation for the next phase


7. Extend your test user group with further participants / departments: Now, you can act iteratively and repeat the relevant steps from before


8. Finalize refactoring and clean up: After the previous step, where you could extend the test group as long as you would like to do (dependent on risk estimation, personal security feeling, time), you have to remove the test spies. At this moment, you do not record any test data. Moreover, you have to activate the new logic for all user

Final Result

• Classes with clear defined responsibilities: Better to understand the coding


• Modularization allows testing with ABAP Unit: Fearless change of existing code


• Further development in parallel mode possible due to separation of concerns into multiple OO classes


• Also an additional benefit: Eases conversion to S/4HANA due to a better code base, because of automated tests, understandable coding and elimination of redundant code

 

What do you think? Do you have other ideas/best practices? Please share your remarks in the comments. Thanks a lot.

[custodio_deoliveira]

Hi Stefan,

 

Very nice guide, thanks for sharing. I just think all of this is not really a refactor, I'd call it a rewrite.

 

Cheers,

Custodio

[vonglan]

Hi Stefan,

that is interesting: 13 likes, but only 1 comment so far.

It seems the topic is regarded as important, but there is not much to say/comment about it.

During our last upgrade, I refactored the sales order user exits (MV45AFZZ) in our system.

We chose to do this during the upgrade, because this minimizes the amount of additional (manual) testing.

What I did was simply to transfer all the coding from FORMs in INCLUDEs to methods in global classes.

So we got rid of the global variable access, and the obsolete language constructs that are allowed in non-OO code. I also solved all ATC priority 1+2 findings.

I did not add any unit tests. And I kept all the routine and parameter names, and most of the comments, like they were, so all developers that previously worked on the exits could still find their way around.

One advantage is that when you have to add or change something in those exits now, the risk of unwanted side effects is smaller because there are no global variables any more.

Another unexpected advantage is, that we can now transport small changes to the exits during working hours without this leading to MISMATCH dumps (as long as the changes are class-internal and do not affect the public section).

Greetings,

Edo

[kay_streubel]

Very good summary, thank you! This definitely supports the "Boy Scout Rule".

I think this might make the topic more accessible and move it from "impossible, we can never do this" to "well, why don't we try?"...

 

 

 

 

[BaerbelWinkler]

Hi Edo,

looks as if I’ll have to pick your brains a bit about this one of these days as we “should” also do something comparable with this exit/enhancement! Unfortunately, we haven’t made any progress since I asked – and closed – my related question in March 2017……

Cheers

Bärbel

[joao_sousa2]

I don’t think you will in general be able to do that kind of refactoring . It’s too large a change for an activity that is mostly likely viewed as nice to have.

It’s correct, I just don’t feel it’s realistic . A more affordable refactor will just remove individual method calls from the include into classes so that when you change a line of code you don’t have to transport the entire include (and reduce locking problems). Thats easier to explain to management , not so easy to explain all the test code, business class refactoring, etc.

[mmcisme1]

I call it a rewrite as well.  During an "upgrade" - we didn't bring everything over to S/4 nor did we rewrite what we did.  Time lines are so very important.  We found that we really had to work hard to get the information into our new system for training.  Normal issue came up that needed addressed.

As Joao Sousa said above, I don't think it is realistic.  It's just too much changing.  Then once we start to learn to integrate the new tools that HANA brings us, another rewrite would have to happen.

I don't disagree that it would be nice to have.  However, that's where it would fall in priority, nice to have.

[Sandra_Rossi]

What's the difference between refactor and rewrite?

[nothafts]

Hi Custodio,

 

Thanks for your feedback.

I would prefer the term "refactoring", because "refactoring" is for me an iterative approach to restructure my code. The approach described here, is a "complex" refactoring, because also the structure would be changed as a final result - all this wouldn't be happen at once, but in small steps.

The term "rewrite" would mean for me to start with a blank page. Of course, you also can do this, but that seems to me very risky and hard to meet the requirements from the last decades w/o having enough tests.

 

Kind regards,

Stefan

[nothafts]

Hi Edo,

 

Thanks for your feedback and sharing your experiences. Basically, that was also my idea to extract the logic in to separate classes 🙂

Maybe, one additional questions: How could you make sure that your changes were valid? Did you have some test users or was it at risk?

 

Thank you.

 

Kind regards,

Stefan

[nothafts]

Hello Joao,

 

Thanks for your remarks. I understand your concerns - yes, the described approach is complete guide to get rid of the monolith and maybe you are not allowed to do all of that. Surely, you can use parts of it, like just extracting some logic into classes. But, how do you know, that your functionality still works? How do you react on code changes?

It always depends what you want to achieve: If you just want to reduce the locking problems and missing tests is not an issue, then you just can extract/introduce classes. IMHO missing tests are always an issue, if you want to change your software afterwards.

If you cannot change your software anymore (and w/o interrupting your production), I would not consider that as a "nice to have". Unfortunately, it is sometimes hard to explain that to the management, till it is too late.

[nothafts]

Hello Michelle,

Please have a look at my answer above 🙂

I'm not sure, if I would compare the transformation from R/3 to S/4HANA with the big includes in customer systems. Moreover, for SAP it is not so easy to record the appropriate test data with all the variants you have. Maybe you could go for your internal tests and demos as a base in order to make sure your functionality still works.

I would like to know, how would you make sure that your changes wouldn't affect the customer systems? Or do you really do a rewrite and substitute the old logic/tool with the new one after you think you are done?

Like I wrote above: It is so long a "low priority/nice to have" till it is too late - code needs to be changeable (in an efficient matter of time)...

[mmcisme1]

Sigh.  I wish we had the time.  What we did was pull over our SAP that we needed.  We knew it was not going to be in SAP because it was custom to us.  Or when we were testing we found out that we needed something.

So no rewrite for us – most of the time.  It was a customer solution that was outside of SAP.  The hooks did need to change for where it went into our SAP system.

The rewrites were ones that took advantage of our new system.  Those were done prior to our final push.  And yes, they were rewrites.  There is a lot of differences between 4.6C technology and NW 7.5.  Some of them went really  fast.  Most of the rewrites were done by consultants that knew the new system better.  However, some of them needed to be fixed later.   When you rewrite something you are bound to do something wrong.  But we didn’t take much with us.

Testing…  Testing and more testing.  The complete system was tested as it was basically a new install.

Lots of work – but it was interesting and fun getting the new technology.

And what we brought over – we could rewrite it.  BUT the code we brought over is critical code.  Not changing it is probably the better approach until we get the time to really test it.

I love what you have here, and hope everyone can do that.  It would have been great if we could have.  On the plus side we have a LOT less custom code.

Oh and I would consider us small to medium - not a large company.

[vonglan]

Hi Stefan,

just noticed that you're also in the DSAG ATC author team 🙂

Regarding testing: we did this as one part of our last system upgrade (suggested by the development team). During the upgrade, we have a big (manual) business user test anyway, so that was our safety net.

Cheers,

Edo

[custodio_deoliveira]

Hi Sandra,

 

To me the biggest difference is, refactoring is a development technique, not a project. It happens in an ongoing basis. If you need permission and budget to do so, you are in a project (or work order or whatever) and it is definitely not refactoring.

With all that said, I really like the approach, very detailed and I will definitely consult it when/if I need to rewrite 🙂 a monolith.

 

Cheers,

Ciustodio

 

PS: There are obviously a lot on this subject on the web. Two of my favorites are https://techbeacon.com/app-dev-testing/rewrites-vs-refactoring-17-essential-reads-developers and https://blog.ndepend.com/rewrite-or-refactor/

 

[Sandra_Rossi]

Thank you for the detailed explanation and links. I was not aware of the importance of what is behind those words, although I understand "refactor" in TDD. Maybe the term "restructuring" as used in the first article (among other possible terms like "redesigning", "reworking", ...) is better than "rewriting", if one considers that "rewriting" is creating code from scratch without looking at the old code.

[matt]

Refactoring is rewriting the code without changing the logic or logical flow. E.g.

• Renaming variables.


• Adding modularisation by breaking up a method into submethods.


• Ensuring the correct visibility of variables - such as ones not really global


• Extracting an interface from a class, and define all instance variables with reference to the interface


• Changing the code so it can be ABAP unit tested. (Putting it in a test harness).


• Changing a function module controlled ALV to a CL_SALV_TABLE.

But there's no hard line, I think. It's a continuum. On the left, definitely refactoring, on the right, definitely rewriting.

Where I've had to enhance an existing FORM based programs, I've shifted the FORMs into methods, and then enhanced. That's refactoring at first.

When I've had to change the logic of an existing FORM based programs, I've just rewritten it.

[matt]

I have recently rewritten an ABAP Objects program, where correct design was lost. I used the original to define the functionality, then used TDD to get that functionality in the new program.

We were able to test the new and make sure we got the same results as the old. Then I fixed a few issues and added new functionality to the new.

The key thing was that we did have enough tests.

[matt]

"And I kept ...  most of the comments, like they were, so all developers that previously worked on the exits could still find their way around."

I've often been able to remove comments by encapsulating the code the comment refers to in a suitably named method.

* Now we flibble the autofrumps

LOOP AT blah ...

  flibble.

  flibble.

ENDLOOP.

becomes

flibble_the_autofrumps( blah ).

[vonglan]

Hi Matthew,

I like that approach.

However, due to time constraints (I had to transform those 10,000 lines between the "normal" upgrade tasks) and again not to completely alienate previous developers, I only did this for recognizable chunks of at least 50 lines of code.

Best regards,

Edo

[matt]

"...not to completely alienate previous developers".

Hmm. I've never been concerned about that... 😄

[pjl]

Hi Stefan,

thank you for you detailed description of this process. I think, whenever you get the chance, the money and the time from your business to do such a project it should be done.

However this is rare. I usually try a more lightweight aproach, so whenever I need to enhance a feature or have to add a feature to this kind of spaghetti monster, I will implement this feature using unit tests in its own classes. And than when I am confident about it's function I will call it from within the monster. (sure this is not always as smooth).

And also If possible I will try to fix a feature individiually by moving it into an extra class structure and then just call it again from the old code.

So the old code gets cleaned up step by step, feature by feature. It takes its time, but I dont need extra budget.

And also one of my biggest problems is, I don't know what this old code should be doing. It is like 80% is 20 years old and nobody knows why it has been written and even if it is correct! So it turns out to rewrite that feature when new requirements are clear is a good way too.

For me real refactoring is only possible if the original code is understandable enough, and I know what it does. If not I tend to rewrite things.

Cheers,
Peter

[Jelena]

I’m a little late to the party here but thank you very much for sharing this, Stefan! Such an interesting subject and nice blog.

As others noted, it must be very fortunate to be allowed time/budget for such project. Where I work, it’d be impossible to do something like that because everything needs to be approved by the business and the business folks don’t want to approve anything that has no monetary value for them. And if, on top of that, we’d need their help with testing or if any risk is involved then it’d be a hard “no”. The arguments like “we spend time now so that we don’t spend even more time later maintaining bad code” don’t work when the business tends to be focused on the current quarter.

So instead we have to resort to “rogue refactoring” in baby steps rather than a “big bang” approach. Deleting old code, renaming, nip and tuck here and there, EC check, etc. But I’m happy to see that we are moving along the same path that you’ve outlined. And this blog gives me a lot of food for thought on how we can do this even better. So, again – thank you for sharing!

P.S. You should include Twitter account in your SCN profile. 🙂

[Private_Member_7726]

Hear, hear…

When I’m faced with a task of fiddling around in a «catastrophe» module pool and have the luxury of spending more than 3 days on a business «problem», I attempt to «refactor» as much of it as reasonably possible to a haphazard collection of local singleton helpers, with a two-pronged objective:

• eliminate as much of global state as possible, rename (yes, to our version of g* prefixes convention, of course ), make «headerlinesless» and encapsulate access to the rest of it (except the absolutely «necessary» TABLES — meaning, I don’t need to or want to touch that ugly DYNPRO)
• identify and remove dead code.

That alone usually suffices to:

• get a reasonable understanding of what the hell is even going on there (and it’s usually not a «rocket science» if it’s familiar «problem domain»);
• establish playing field where things can be changed and improved without looking forward to a mini-avalanche of tedious to fix bugs;
• get rid of that feeling of helplessness, despair and rage

No «unit tests» – they are a «waste of time» and «nobody will understand» what the hell I’m even talking about when they inquire when can they finally have their «thing» or bug correction ready for what they were made to believe to be «test». Change a little, try if the thing still seems to work, make it work if it doesn’t, repeat…

No freaking «reuse» concerns – I wasn’t, as a rule, asked to provide «reuse», but to fix or add this or that… The only exceptions (things that go into global singletons/constant-cum-related-reusable-types interfaces are):

• customizing entity read accesses;
• reusable constants;
• if there is time – stateless «Functions facade» classes for SAP standard function calls; one class per FUGR, one method per FUBA with RETURNING, if at all identifiable, the «main result» of function; parameters renamed to our prefix convention; refactor the calls to newly established «Functions facade» in the rest of custom code, if there is time…

It basically amounts to little more than fixing what the procedural programmer neglected to do in the first place: a) establish and maintain over time reasonable code modularisation; b) use the freaking parameters to pass the data between modularisation units that support them (don’t be lazy).

[Jelena]

Haha, "get rid of that feeling of helplessness, despair and rage" - that's high on my list of achievements! 🙂

I know some (quite possibly, many) people feel skeptical about this. I'm also a big fan of "don't fix what ain't broken" but if a code cannot be easily understood and full of BINARY SEARCH because a global table (with header line, of course) is not defined with the proper type then it IS broken.

Also bad code leads to more bad code due to a "broken window syndrome", as mentioned in this discussion.

[hardyp180]

I just spent 18 months with a colleague dealing with a monolithic monster with all the bad traits mentioned in this blog, and more.

We did a total rewrite, started from scratch. We had the budget as we had a far sighted CIO.Not everyone has that luxury, in fact 99.9999999% of programmers do not, so you are bound to come across such a programmer eventually.

The problem - as mentioned in "Clean Code" and many other places is that the monsters get bigger and more complicated and are held together with string and bogies, and the bigger they get the more likely they are to fall apart - in our case a change to Poland could break something to do with the UK.

It was a nightmare with six countries on SAP. The problem would increase exponentially and we had a target of 50 countries.

The best quote on this was when the development manager asked one of the expert programmers - "Would this approach (a OO re-write) make adding the 10th country easier or more difficult?"

He thought about it and said :it would make it possible"

Robert Martins position is that it does not have to be this way - programs rotting with time - you can turn this on its head and design things so they get better with time i.e. more stable, more resistant to change. This is, in my mind, what unit testing and the separation of concerns are all about.

Let us take one DYNPRO thing I had to deal with this very day. A huge problem with programming is that you often have assumptions and most assumptions are invisible and if you do not know the assumptions you can change something and break things.

In a DYNPRO you have an ON REQUEST for a field which calls a MODULE which (due as advised by SAP) calls a FORM routine.. So the FORM routine has the invisible assumption it was called from a certain screen and the invisible assumption that a user has just entered or changed a value in a certain field. The FORM routine does not know that, and of course someone could look at it and say "that is just what i want" and call it from a place where none of the assumptions are true.

In classic DYNPRO you could use comments to list the assumptions but that is horrible and I bet no-one ever does.

At the very minimum you could make a change to a method and since the parameters are typed the call could be REDETERMINE_WIDGETS( WHEN_NEW_XYZ_VALUE_IS = G_THING

AND SCREEN IS = C_THING_SCREEN ).

That is about as good as you are going to get from a DYNPRO but at least you have made your assumptions clear in the code and it is a completely non intrusive change (as you have not changed the logic in the routine).

Cheersy Cheers

Paul

[hardyp180]

BTW the total re-write in an OO way had no business logic at all in the DYNPROS which were encapsulated in function modules.

The DYNPRO problem is the second half of my comment was in a program I could not totally re-write so had to make a surgical enhancement.

[joachimrees1]

Wow, a very good read, thanks for sharing!