Guru: Refactoring into Routines
May 7, 2018 Ted Holt
In RDi and Refactoring, I illustrated the process of refactoring by taking code of a very old style and converting it little by little into something modern. I promised to write more about the subject, and today I fulfill that promise.
The things I did in that first article — removing indicators, removing the COMP op code, removing GOTO, and renaming variables — are great, but they are not the only refactoring techniques. One of the best ways to refactor is to create new routines or improve existing routines, especially routines that can stand alone.
To illustrate, I’ll begin with the next-to-last version of the discount calculations from the previous article.
FREFACTDT IF E K DISK FQSYSPRT O F 132 PRINTER * D DisPct s 3p 2 * C READ REFACTDT LR C *INLR DOWEQ *OFF * C select C when discod = 0 C eval dispct = *zero C when cuscls = 'C' C eval dispct = *zero C when cuscls = 'A' C eval dispct = 0.08 C when (discod = 1 and qty >= 12) C or (discod = 2 and qty >= 24) C eval dispct = 0.05 C when cuscls = 'B' C eval dispct = 0.04 C other C eval dispct = *zero C endsl C* C EXCEPT DTL C* C READ REFACTDT LR C ENDDO OQSYSPRT E DTL 1 O KEY O CUSCLS +003 O DISCOD +003 O QTY 1 +003 O DISPCT 1 +003
Let’s make a subroutine of the code that calculates a discount factor. In this case, that’s a single SELECT structure. (If this were a real-world example, there would be more to it than that.)
C READ REFACTDT LR C *INLR DOWEQ *OFF * C exsr CalcDiscount C EXCEPT DTL C* C READ REFACTDT LR C ENDDO C CalcDiscount begsr C select C when discod = 0 C eval dispct = *zero C when cuscls = 'C' C eval dispct = *zero C when cuscls = 'A' C eval dispct = 0.08 C when (discod = 1 and qty >= 12) C or (discod = 2 and qty >= 24) C eval dispct = 0.05 C when cuscls = 'B' C eval dispct = 0.04 C other C eval dispct = *zero C endsl C endsr
Has that accomplished anything? Yes, it has, because it leads to another improvement — conversion of the subroutine into a subprocedure.
When converting to a subprocedure, the main task is to eliminate all global variables. To do so, change each variable to a parameter or a local variable. You do not have to rename them, because the compiler can distinguish between global and local variables.
Here’s the subprocedure. Notice that customer class, discount code, and quantity are input parameters, and discount percentage is a local variable.
dcl-proc CalcDiscount;
dcl-pi *n packed (3:2);
CusCls char (1) const;
DisCod packed (1) const;
Qty packed (5) const;
end-pi;
dcl-s DisPct packed (3:2);
select;
when discod = 0;
dispct = *zero;
when cuscls = 'C';
dispct = *zero;
when cuscls = 'A';
dispct = 0.08;
when (discod = 1 and qty >= 12)
or (discod = 2 and qty >= 24);
dispct = 0.05;
when cuscls = 'B';
dispct = 0.04;
other;
dispct = *zero;
endsl;
return DisPct;
end-proc;
Also, since this routine calculates a single value, we can make it serve as a function. That is, it returns a value that can be tested or assigned to a variable, as any other value can.
dcl-pi *n packed (3:2);
Here’s the subprocedure with the calling code. Notice that the main routine uses EVAL to invoke the subprocedure and assign the returned value to global variable DisPct.
C READ REFACTDT LR
C *INLR DOWEQ *OFF
*
C eval DisPct = CalcDiscount (CusCls: DisCod: Qty);
C EXCEPT DTL
C*
C READ REFACTDT LR
C ENDDO
. . . O specs omitted . . .
dcl-proc CalcDiscount;
dcl-pi *n packed (3:2);
CusCls char (1) const;
DisCod packed (1) const;
Qty packed (5) const;
end-pi;
dcl-s DisPct packed (3:2);
select;
when discod = 0;
dispct = *zero;
when cuscls = 'C';
dispct = *zero;
when cuscls = 'A';
dispct = 0.08;
when (discod = 1 and qty >= 12)
or (discod = 2 and qty >= 24);
dispct = 0.05;
when cuscls = 'B';
dispct = 0.04;
other;
dispct = *zero;
endsl;
return DisPct;
end-proc;
Now that the subprocedure stands on its own, we can rename those short identifiers to improve the readability. This is another place where RDi comes in handy.
In the previous article, I showed how to use RDi to rename a variable. What I didn’t tell you is that RDi is smart enough to rename a local variable without renaming other variables of the same name. In the following version of the source code, I renamed DisPct, CusCls, DisCod, and Qty in the subprocedure. RDi did not change those names in the main routine. Had it done so, we’d have had a mess, because the six-character-or-shorter names come from the input file.
D DisPct s 3p 2
*
C READ REFACTDT LR
C *INLR DOWEQ *OFF
*
C eval DisPct = CalcDiscount (CusCls: DisCod: Qty)
C EXCEPT DTL
C*
C READ REFACTDT LR
C ENDDO
. . . O specs omitted . . .
dcl-proc CalcDiscount;
dcl-pi *n packed (3:2);
inCustomerClass char (1) const;
inDiscountCode packed (1) const;
inQuantity packed (5) const;
end-pi;
dcl-s DiscountFactor packed (3:2);
select;
when inDiscountCode = 0;
DiscountFactor = *zero;
when inCustomerClass = 'C';
DiscountFactor = *zero;
when inCustomerClass = 'A';
DiscountFactor = 0.08;
when (inDiscountCode = 1 and inQuantity >= 12)
or (inDiscountCode = 2 and inQuantity >= 24);
DiscountFactor = 0.05;
when inCustomerClass = 'B';
DiscountFactor = 0.04;
other;
DiscountFactor = *zero;
endsl;
return DiscountFactor;
end-proc;
The hardest part is far behind us.
Now that the discount routine is in a subprocedure with no external dependencies, we can move it into a service program. We create a source physical file member for the module.
ctl-opt nomain;
/copy prototypes,SALESRTNS
dcl-proc CalcDiscount export;
dcl-pi *n packed (3:2);
inCustomerClass char (1) const;
inDiscountCode packed (1) const;
inQuantity packed (5) const;
end-pi;
dcl-s DiscountFactor packed (3:2);
select;
when inDiscountCode = 0;
DiscountFactor = *zero;
when inCustomerClass = 'C';
DiscountFactor = *zero;
when inCustomerClass = 'A';
DiscountFactor = 0.08;
when (inDiscountCode = 1 and inQuantity >= 12)
or (inDiscountCode = 2 and inQuantity >= 24);
DiscountFactor = 0.05;
when inCustomerClass = 'B';
DiscountFactor = 0.04;
other;
DiscountFactor = *zero;
endsl;
return DiscountFactor;
end-proc;
And one for the prototype. . .
dcl-pr CalcDiscount packed (3:2); inCustomerClass char (1) const; inDiscountCode packed (1) const; inQuantity packed (5) const; end-pr;
Then we use CRTRPGMOD to create the module and CRTSRVPGM to create the service program.
CRTRPGMOD MODULE(MYLIB/SALESRTNS)
SRCFILE(MYLIB/QRPGLESRC) SRCMBR(SALES)
CRTSRVPGM SRVPGM(SALES) MODULE(*LIBL/SALES)
What’s next? That depends. What would you like to do? Here are some ideas:
(1) You could expand the service program by adding a subprocedure to calculate the price a customer has to pay for an item.
dcl-proc CalcCustomerPrice export;
dcl-pi *n packed (5:2);
inCustomerID packed (7) const;
inItemID char (6) const;
inQuantity packed (5) const;
end-pi;
dcl-s CustClass char(1);
dcl-s Price packed(5:2);
dcl-s DiscountCode packed(1);
exec sql select CusCls into :CustClass from refactcus
where Account = :inCustomerID;
exec sql select Price, DisCod
into :Price, :DiscountCode from refactitem
where ItNbr = :inItemID;
return (1 - CalcDiscount (CustClass: DiscountCode: inQuantity))
* Price;
end-proc;
(2) You could create an SQL function that returns the unit price that a customer pays for an item.
create or replace function CustomerPrice (dec(7), char(6), dec(5)) returns dec(5,2) language rpgle parameter style general deterministic reads sql data returns null on null input program type sub no final call external name 'MYLIB/SALES(CALCCUSTOMERPRICE)'
And use it in queries.
select OrderNbr, Customer, Item, Qty,
CustomerPrice (Customer, Item, Qty) as UnitPrice
FROM salesdata
(3) You could use these routines in Web services.
And so on and so forth.
Systemwide, the CalcDiscount routine would handle all customer discounts. A change in policy would require only a change in one routine in a service program.
And to think it all began with some spaghetti code written in an ancient version of RPG.
Custom software is a valuable asset and should not be thrown away without good reason. Refactoring puts new life into old code and carries it into the future.
I love the step by step approach Ted – nice.
Personally I would have taken the opportunity to replace the SELECT operation with IF/ELSEIF. Reason is that my brain thinks that SELECT should reference different conditions of the same variable. In your example multiple variables are being tested and that breaks my rule and the “right” choice is IF/ELSEIF. I find that if I stick to this approach, reading the code is easier as I know in advance what to expect.So my version (assuming no typos!) would be:
If ( inDiscountCode = 0 );
DiscountFactor = *zero;
ElseIf ( inCustomerClass = ‘C’ );
DiscountFactor = *zero;
ElseIf ( inCustomerClass = ‘A’ );
DiscountFactor = 0.08;
ElseIf ( inDiscountCode = 1 and inQuantity >= 12 )
or ( inDiscountCode = 2 and inQuantity >= 24 );
DiscountFactor = 0.05;
ElseIf ( inCustomerClass = ‘B’ );
DiscountFactor = 0.04;
Else DiscountFactor = *zero;
EndIf;
Apologies for the lack of code formatting – indents were there when I typed it honest!
Very good! I do have a suggestion. It’s not a big deal, it’s really a personal preference of style that I think adds.
Use the names throughout – for example:
dcl-proc CalcDiscount;
dcl-pi CalcDiscount packed (3:2);
inCustomerClass char (1) const;
inDiscountCode packed (1) const;
inQuantity packed (5) const;
end-pi CalcDiscount;
….
end-proc CalcDiscount;
And:
dcl-pr CalcDiscount packed (3:2);
inCustomerClass char (1) const;
inDiscountCode packed (1) const;
inQuantity packed (5) const;
end-pr CalcDiscount ;
I do this with my DCL-DS as well:
DCL-DS dsname QUALIFIED;
xyz char(5);
zyx char(5);
END-DS dsname;
This just more self documenting. Also – creating the prototype then is simply copying the DCL-PI through END-PI statements, change the PI to a PR and you’re done – ie.
dcl-pi CalcDiscount packed (3:2);
inCustomerClass char (1) const;
inDiscountCode packed (1) const;
inQuantity packed (5) const;
end-pi CalcDiscount;
becomes:
dcl-pr CalcDiscount packed (3:2);
inCustomerClass char (1) const;
inDiscountCode packed (1) const;
inQuantity packed (5) const;
end-pr CalcDiscount;
It takes 4 seconds to create your PR statements.
When scrolling through the code it’s clear what the END-PROC belongs to because it has the name of the procedure. The more you self-document the better.
I realize that this isn’t within the scope of the article – but it does, IMO, help with a philosophical style.
The coding style should, IMO, have the idea of the code being self-documenting as much as possible.
As you have coded – subprocedures should have one entry point with all parameters defined with one exit point. what is retuned is known. No external dependencies – ie no global variables.
BTW – there would be indentation but the reply doesn’t have that option – as far as I can tell…