The Amityville Programming Horror - Part 2

[hardyp180]

The Amityville Horror – Part 2

What do you do when you have to change a program that is so badly structured the merest touch will smash it like glass?

In a prior blog I outlined the general problem:-

https://blogs.sap.com/2017/02/17/the-amityville-programming-horror/

Now is the time to have a look at the code in detail, see what is wrong – which as we will see is not that difficult as virtually everything is wrong – and go about fixing it in a non-destructive manner.

Rather than just wholesale poking about, the obvious place to start is the functionality that I am supposed to be changing. I can’t just open up the program as and when I feel like it, and start to fix it up, I need some sort of excuse.

In this case, the requirement is fairly simple, and twofold.

• First of all, add two new selection options, for payer and another partner function


• Add another database read on yet another table, to get a status field for the delivery (of monsters)

Now, I imagine in 99% of organisations, maybe a higher percentage, what you do is make the exact change(s) required and ignore the fact the program is a horrible mess. The fact that you are in fact making it worse is neither here nor there, as long as the requirement is met, really fast.

That sounds right. Time (and money) is always tight and getting tighter. We want to change this as quickly as possible, do we not?

Moreover, when I suggested to my colleagues that we might try Robert Martins “Boy Scout Rule” i.e. always leave the code base tidier than it was when you found it”, I got quoted another Boy Scout rule in return namely “Be Prepared” i.e. “Be Prepared to get sacked for changing something you were not explicitly told to change”.

That is always a killer – if you find an obvious bug, do you fix it, even if that is not what you are actually supposed to be doing? In some companies you could get sacked for that. I find in Australia and Israel I have no problems, in Germany it is a very different kettle of fish.

Now I don’t want to get anyone sacked, so if you work for an organisation where you are not allowed to use your own initiative to make things better, on pain of death, then please stop reading now, but before you do please email me your companies name, so I can buy shares in their competitors.

Living on a Payer

There is nothing easier than adding a new selection option to a “Type 1” executable program that produces and ALV list. That is, in a procedural program. For object orientated programs you tend to have to go all round the houses, to make sure the value the user enters on the selection screen gets to the classes that need to know about that value or values.

Let us just say I had one requirement only – add a selection on the “payer” partner function. That should just be two lines of code that need to change – add a new SELECT-OPTION and then change one SELECT statement. As I said, in an OO program you might need to change a few more lines to get the selection criteria from Lands’ End to John O’Groats, but still not the end of the world.

This is a procedural program so I should be finished in ten seconds flat. However after those ten seconds of work, I run the report as a test and the change does not seem to work at all. How can that be? A simple change like this – what could go wrong?

The first thing I notice is that the name of the “payer” is wrong. Since I am now selecting by “payer” that is somewhat worrying. The program has been getting the payer name since time immemorial; I have not changed that part, only the database selection. How could that stuff things up?

The obvious test is to run the report in production and see if the payer name is correct. Well, it is not. This clearly has not been working, possibly since day one, and no-one has noticed. So, my change in development has worked – only records with the correct “payer:” are returned, it is just that the name is wrong.

I could just let it go and say “all done” but as sure as eggs are eggs when someone tests this in development or QA they are going to tell me the payer name is wrong, and it must be my latest change that has “broken” the program, regardless of the fact it does not – and has never – worked properly in production. Quite possibly the person who is moaning to you now about the fault was the same person who signed the program off many years earlier as working perfectly and OK to go to production.

This is actually good – you now have carte blanche to fix an existing bug because “your current change has broken it”.

Dr.Seek and Mr.Hyde

So, if the payer name is wrong, I have to find out where it is getting set – and this is easier said than done. Although just a report this program has ballooned out so much over the years it has routines all over the place. There are various ways to pin this down – do a “find/replace” search in the ABP editor, or do an ST05 trace and looks for reads on KNA1.

.I find the problem code in the end – in a routine called “GET TEXT DESCRIPTIONS” which gets the text names of a whole bunch of unrelated fields, based on their key values. Here is the first problem – if it takes ages to find the place you need to make your change, if you have to make another change in a years’ time, it is going to take ages again to find that code, just as it did today.

That would be a sign to rename the routine – say “GET PARTNER DESCRIPTIONS”. If some f that routine has nothing to do with getting the partner descriptions, then it should be in its own routine, describing what the code does more accurately. You end up with more routines than before, but with better names so in the future you can zoom in on the problem area in a much faster manner. When I say “routines” I really mean “methods” as opposed to ”form routines” but as this is a procedural program I am stick with routines at least at the start.

Here is the code with the problem. Can you spot the mistake?

"Get bill-to and name.
SELECT kunnr
INTO <gs_alv_output>-kunre
FROM vbpa
WHERE vbeln EQ <gs_alv_output>-vgbel
AND parvw EQ 'RE'.

"Get Payer and name.
SELECT kunnr
INTO <gs_alv_output>-kunrg
FROM vbpa
WHERE vbeln EQ <gs_alv_output>-vgbel
AND parvw EQ 'RE'.

This simple example illustrates about ten billion zillion points, all at once.

In SAP terms the abbreviation “RE” means “bill-to” and “RG” means “payer”. The original programmer even built that into the variable names i.e. KUNRE and KUNRG.

Paster Man, Paster Man, who wanna be a Paster Man? (Boney M)

The problem came when doing the ever popular “cut and paste”. Clearly the second SELECT statement was copied from the first, and the variable name was changed from KUNRE to KUNRG but the selection option on PARVW was not changed…. hence the error.

This also fulfils the “make wrong code look wrong” principal – the names of the variables were such they clearly indicated what they were looking for i.e. an RG instead of an RE, but still no-one noticed. An argument code be made for using constants, or some other way of arriving at the value “RG” and also renaming the target variable so you get the following:-

"Get Payer and name.
SELECT kunnr
INTO <gs_alv_output>-payer
FROM vbpa
WHERE vbeln EQ <gs_alv_output>-vgbel
AND parvw EQ gc_bill_to.

When you look at this code, it is a lot more obvious something is wrong.

The two points we can learn from this is to replace literals with meaningful names, and more importantly every time you feel like cutting and pasting, think to yourself – “can I move this code into its own routine and then parametrise the differences?” In ABAP in Eclipse this is an incredibly easy thing to do.

The obvious counter argument runs along the lines of “we are only talking five lines of code here” so it seems like an enormous effort to create a new routine just with those five lines inside it. After all nothing could possibly go wrong when copying those five lines of code!

Well – it can, and it did. So I would say every time you have the urge to do a cut and paste of some code think to yourself “Is this going to come back and bite someone in the future? More importantly is it going to come back and bite ME?”

More PASTE less SPEED

What we see here is the false economy that plagues programming – you do something like cutting and pasting because it seems the fastest way to get the job done, and then a year down the track you have to spend ages trying to work out what was wrong, because you took the easiest way out in the first place, Since you only spend 5% of the time creating programs, and 95% of the time changing/enhancing them, techniques that help you in the 95% are going to be much more worthwhile, even if it makes the 5% slightly more painful.

Dick Performance Tracey

Now I have fixed the code – i.e. the “RG” variable now gets the “RG” value from the database; it is time to re-run the report. Things look a lot better this time. Now I do an ST05 performance trace on the report to make sure my adjusted database read still uses the primary key. What do I see when I run such a trace?



That is not good. In essence generally no program should be doing so many database reads that the message above is triggered. It is a sign of lots of little database reads in a loop or some such.

Next time we shall have a look at how to get the program in such a state that it no longer causes the ST05 trace to have a nuclear meltdown.

Cheersy Cheers

Paul