--- here we have located customer numbers that we want to purge in other files. First we are reading the customer master, then we see if it the customer number exists, in the order history or the Invoice history. If Not, then we want to purge this customer from Customer master as well as 2 other files.
However in the second file, if the customer number has an 'A' or 'C' in a marketing column, and it's after 2007, we don't want to purge this one from ANY of the files.
SO I made code that before it writes the customer record to a save/hold file and deletes, it gets back a flag, that yes, this is ok to remove.
C IF PUGFIL = 'Y' AND
C ACENT# <> ACENT#_OLD
c EXSR CHKCUS_SR
c ACFLAG IFEQ 'N'
C WRITE TRCMASRR
c* delete arcmasrr
c CHKCUS_SR BEGSR
c eval ACFLAG = ' '
C ORHKEY SETLL dRCST1
C ORHKEY READE dRCST1
* If the order entity is found, write the rec into VRCSTKBI file
C DOW NOT %EOF(dRCST1)
c if BICOTC <> 'A' AND BICOTC <> 'C'
C WRITE VRCSTKRR
c EVAL ACFLAG = 'N'
c endif
c if bicotc = 'A'
c if BISTPD < 20070101
C WRITE VRCSTKRR
c EVAL ACFLAG = 'N'
c endif
c endif
c if bicotc = 'C'
c if BISTPD < 20070101
C WRITE VRCSTKRR
c EVAL ACFLAG = 'N'
c endif
c endif
c acflag ifeq 'N'
C EXSR CHKADR_SR
I'd write a function (sub-procedure). Local variables make code like this easier to work with because you don't collide with variables in the mainline. I find that creating a function helps me organise my thoughts, and with organised thoughts I write better code. 'Better' is of course subjective, but here I mean easier to understand and most importantly, easier to change without breaking something else in the process.
The variable names... ooh. Use longer names - names that make sense. ACFLAG will make your life worse the next time you have to look at this (maybe in a year? Seven years?) I much prefer Benny's do_purge - it says what it intends. It could have been an indicator variable to really hammer home the point that it's a yes/no decision point, but it's definitely easier to understand if do_purge = 'Y'than it is to understand if acflag = 'N'. Negative logic adds to the problem. The subroutine suffers from the same cryptic naming convention. Check customer. Check it for what? What's the business functionality being implemented? If it can't be easily described by the name, it's too complex - doing too many things. Is the business function 'Check for active customer'? Name the subroutine that way (or better still, write the function name that way). Your mainline could become
if custIsInactive(customerID);
exsr purge_customer;
endif;
Comments. Benny did a good job with what he had to work with. The original code has exactly one comment, and it's almost completely unhelpful. We can see that the order entity is found - that's what if not %eof() means. And we can also see that we're going to write a record. But there's no explanation of why these actions are important, desirable and useful. Here's something that helps me a lot. Write the comments first. Not pseudocode! The worst comments in the history of the universe look like this:
// if X > 10, reset y
if x > 10;
y = 0;
endif;
That comment just distracts the eye from the code. White space would be better. Simple code requires no comment. More complex code always benefits from a comment that explains the intent. What sort of comments would help this code? In English, explain why codes A and C are important. Maybe it's because
// don't purge customers we've done business with
// exception: if we emailed or cold called them
// and they didn't buy anything in the
// past 6 years, purge them.
if (bicotc = 'A' and bistpd >= 20070101) or
(bicotc = 'C' and bistpd >= 20070101);
do_purge = 'Y';
endif;
I realise the posted code doesn't do this, but from this side of the glass I can't tell if you intended it the way it's written, or it's a bug that we just haven't tripped yet. Comments should clarify intent. Believe me, when the next person gets into this code, he'll be happy to read the plain English reason for codes A and C, and why that specific date is important. Maybe it's the date of a merger or acquisition, and A and C items came from the old division... The code as-is doesn't explain.
If comments are frowned upon (and they are in some places) at least avoid 'magic codes' and 'magic numbers'. How about this:
if (bicotc = OHIO_BIG_TICKET and bistpd >= MERGER_DATE) or
(bicotc = OHIO_LITTLE_TICKET and bistpd >= MERGER_DATE);
do_purge = 'Y';
endif;
Finally, back to the concept of doing one thing at a time. This 'check customer' subroutine clearly does more than 'check' - it's writing records to VRCSTKBI. This looks like a bug to me based on the description of the situation. Based on the setll/reade loop, it appears that the code is looking through an order history file. This subroutine would write to VRCSTKBI for the first 10 items and then the 11th makes the customer ineligible for a purge. But there are records in VRCSTKBI for that customer. Whoops. Very often, we're tempted to bundle multiple I/O operations together in the name of efficiency. I'm here to tell you that after 35 years of doing this, I agree with Donald Knuth:
"We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil."
Think about the business functions.
If you have separate functions for each business operation it would be easier to write, understand test, debug and modify in the future. Write 3 sub-procedures, name them with good names and then deploy them in the mainline:
if custIsInactive(customerID);
record_purged_customerID(customerID);
purge_customer(customerID);
endif;
A function either returns information (custIsInactive) or provides a service (purge_customer). There shouldn't be much business logic making decisions in the 'provide a service' functions, and the function that's serving up information shouldn't be implementing services. Not because mix and match is inherently evil, but because the more things a slice of code does, the harder it is to understand and maintain. We can only keep a small number of things in our active memory, so the ability to abstract out a business function into a single item - a function name - is a very powerful aid to making robust programs.