Dirty RPG programmers need to write Clean RPG Code

Write Clean RPG Code

This morning I was revisiting a program that I wrote last year and looking at my code thinking “Why the hell did I write it like that? #Ugly”. RPG code evolves and lots of code evolution starts in one direction and then branches off, drunkenly in another direction. This means that code can evolve to be quite ugly. I rewrote this code to make it a little more verbose, but much more readable and easier to understand for people that modify the code later. Even Dirty Programmers can write Clean RPG Code is good RPG code. sometimes 🙂

So, I had to add a new sub-procedure and decided to refactor the code to my current (2017) programming style.

The original code looked like this:

// Load *LDA First 
If #SetWebServiceLDA ( Company : User : Stockroom : RtnErrorCode ); 
  // Load *LIBL next 
  If #SetWebServiceLIBL ( DeveloperLib :  RtnErrorCode ); 
  // Check User Authority Next 
   If #ValidUser ( User :  RtnErrorCode ); 
   // Check Stockroom Authority finally 
    If #ValidStockroom ( Company :  Stockroom :  User :  RtnErrorCode );  
    Else; 
      RtnMessage = '*USER/STOCKROOM Error* ' + %trim(RtnErrorCode); 
    Endif; 
   Else; 
     RtnMessage = '*USER ERROR* ' + %trim(RtnErrorCode); 
   Endif; 
  Else; 
   RtnMessage = '*LIBL ERROR* ' + %trim(RtnErrorCode); 
  Endif; 
 Else; 
   RtnMessage = '*LDA ERROR* ' + %trim(RtnErrorCode); 
 Endif;

So, this is a webservice program talking to INFOR SYSTEM21 ERP. You can see the webservice is attempting to

1. Set the *LDA with System21 default values
2. Set the webservice library list from System21
3. Validate the User (the webservice user) is authorized to System21
4. Validate the user is authorized to the System21 Warehouse that it is working with

I know the code is efficient and does it’s job but I can’t help but look at it and think it’s ugly and not that intuitive. So, in the essence of K.I.S.S (Keep It Simple Stupid) programming style, I opted for this simpler/cleaner code as part of my modification:

I added four new variables (what us old RPG3 guys would have defined as indicators in the olden days) to show the status of each subprocedure call.

dcl-s validLDA ind inz(*off);
dcl-s validLibl ind inz(*off);
dcl-s validUser ind inz(*off);
dcl-s validStockroom ind inz(*off);

and then changed the code to try every procedure first and then report on the results:

// Load *LDA First 
validLDA = #SetWebServiceLDA ( Company : User : Stockroom : RtnErrorCode ); 
 
// Load *LIBL next 
validLibl = #SetWebServiceLIBL ( DeveloperLib : RtnErrorCode ); 
 
// Check User Authority Next 
validUser = #ValidUser ( User : RtnErrorCode ); 
 
// Check Stockroom Exist and that User is authorised to it 
validStockroom = #ValidStockroom ( Company : Stockroom : User : RtnErrorCode ); 
 
if not validLibl; 
 RtnMessage = '*LIBL ERROR* ' + %trim(RtnErrorCode); 
Elseif not validUser; 
 RtnMessage = '*USER ERROR* ' + %trim(RtnErrorCode); 
ElseIf not validLDA ; 
 RtnMessage = '*LDA ERROR* ' + %trim(RtnErrorCode); 
Elseif not validStockroom ; 
 RtnMessage = '*USER/STOCKROOM Error* ' + %trim(RtnErrorCode); 
Endif;

Every webservice call should run all four of these subprocedures and I think the code is much more readable for the poor programmers that have to maintain my code in the future… when I’m long gone….

Clear Code is better than clever code

/me rides bicycle off into the distance…