looking for sharper way to code this RPG LE

Question 1

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.

Is the customer eligible to be purged?
Record the customer to be purged.
Purge the customer from daughter files.

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.

Question 2

Buck and Benny have given a lot of good suggestions on ways to improve the RPG code.

Use free format to improve readability
Use longer descriptive names, to clarify what things actually are. (Don't make someone have to decypher a name when you could have just given clear name to begin with)
Use sub-procedures not subroutines. Returning a value from a subprocedure makes it a user-defined function, even better

A procedure should perform one idea. Everything in that procedure should be related to doing that one thing. This is called cohesion. Keep your procedures fairly small and simple. How small. Semour Papert, who was the head of the MIT AI Lab, was interested in enabling young students to program computers to do things that interested them. When he asked one of them how large they thought a procedure should be, the answer he got was "a mind-sized bite".

You want to minimize unnecessary dependencies between procedures, so that a change in one area is less likely to cause issues in another area. This is called coupling.

Inside your loop, notice how many places you check for 'A' or 'C', and you repeat the same block of code once for 'A', then again for 'C'. You could have used IF .. OR.. instead, so that you wouldn't have repeated the block of code, which can lead to maintenance issues in the future. It violates the DRY principle, Don't Repeat Yourself. You can think of yourself as a tidy housekeeper saying, there is a place for everything [line of code], and everything [line of code] in its place.

Now to a minor point. All over the place, I see people use SETLL immediately followed by READE using the same key. Use CHAIN instead. Under the covers, a CHAIN performs the logic done by the SETLL and then the logic for the READE. Some folk think they are saving time by making the READE conditional on a successful SETLL. But what is happening is that with each I/O statement, the compiler generates code to prepare parameters to call an I/O module, calls the module to perform the I/O function, then processes the parameters returned. With two I/O statements you are doing this twice. Let the CHAIN operation handle it for you, and it also has the opportunity to gain some internal efficiencies. Plus your RPG code is now a bit simpler. It's better from every angle.

Get ready for it...

Instead of using the traditional I/O statements, you should use embedded SQL. There are so many reasons that I really dont want to write a whole paper on it here. Just trust me for now. The investment in learning this really pays off.

You'll learn to SELECT, DECLARE and OPEN CURSORS (like an open data path), then FETCH records from the cursor, even FETCH or INSERT multiple records at a time.

Now, the really big thing

The traditional RPG paradigm is generally going through loops, processing one record each time through the loop, often performing additional I/O on other files one record at a time.

SQL enables you to process records as a set in a single statement, over an entire file, or over multiple files. Your RPG code can be reduced and simplified dramaticallly by running through the entire file, plus the other two, in a single SQL statement.

CREATE TABLE QTEMP/PURGING AS
( SELECT c.customer, ...
    FROM Customers c
    LEFT EXCEPTION JOIN Orders o
            on c.customer = o.customer
    LEFT EXCEPTION JOIN Invoices i
            on c.customer = i.customer
    WHERE c.customer not in 
           (select s.customer
              from secondfile s
              where marketing in ('A','C')
                and eventdate > '2007-12-31'
           )
) with data;

DELETE FROM secondfile x
  WHERE x.customer in
          (select p.customer
             from purging p
          );

DELETE FROM thirdfile x
  WHERE x.customer in
          (select p.customer
             from purging p
          );

DELETE FROM Customers x
  WHERE x.customer in
          (select p.customer
             from purging p
          );

Four statements. That's all it takes. No looping. Once you've got the first statement right, the rest is pretty simple.

The CREATE TABLE ... WITH DATA writes the results of the SELECT to the new table. I only show it going into QTEMP because I wasn't sure if you wanted to keep it or not. The LEFT EXCEPTION JOIN says use rows from the table on the left side, that do NOT have rows on the right side matching the search condition. This lets you pick customer records that are not in the order history and not in the invoice file. Once that file is built containing the list of customers you want to purge, then you can use that list to remove those customers from the Customer Master and the other two files.

Question 3

You only need the customer column that's all you take eliminate the three dots that imply your selecting more columns.

Make the purge table unique by adding the distinct keyword then you can use three exception joins that will be much more readable because the analysis doesn't have to shift gears from exception join to not in. The deletes could definitely go faster if the temporary purge table had a primary key on customer.

I would also add in a drop table just in case.

drop table qtemp/purging;

CREATE TABLE QTEMP/PURGING AS
( SELECT distinct c.customer
FROM Customers c
LEFT EXCEPTION JOIN Orders o
        on c.customer = o.customer
LEFT EXCEPTION JOIN Invoices i
        on c.customer = i.customer
left    exception join secondfile s on 
        c.customer = s.customer and
          marketing in ('A','C')
            and eventdate > '2007-12-31'
       )
) with data;

Question 4

Is this what you're wanting?

/free
    if pugfil = 'Y' and agent# <> agent#_old;
       exsr chkcus_sr;
       if do_purge = 'Y';
          write trcmasrr;
          delete arcmasrr;
       endif;
    endif;

    begsr chkcus_sr;
       // Assume we will purge this customer.
       do_purge = 'Y';
       setll orhkey drcst1;
       reade orhkey drcst1;
       dow not %eof(drcst1);
          // If bicotc is A or C and the date is January 1, 2007 or later, do NOT purge and stop checking other records.
          if (bicotc = 'A' and bistpd >= 20070101) or
             (bicotc = 'C' and bistpd >= 20070101);
             // Make sure you change the flag to say NO - DON'T PURGE THIS CUSTOMER
             do_purge = 'N';
             leavesr;
          endif;
          write vrcstkrr;
          // Looks like you are doing more processing here but you don't show the code...
          reade orhkey drcst1;
       enddo;
    endsr;
 /end-free

Or if you want to stick with fixed format:

c                   ifeq      pugfil = 'Y' and
c                             agent# <> agent#_old
c                   exsr      chkcus_sr
c                   if        do_purge = 'Y'
c                   write     trcmasrr
c                   delete    arcmasrr
c                   endif
 *------------------------------------------------------------------
c     chkcus_sr     begsr      
c                   eval      do_purge = 'Y'
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 bistpd >= 20070101) or
c                             (bicotc = 'C' and bistpd >= 20070101)
c                   eval      do_purge = 'N'
c                   leavesr
c                   endif

c                   write     vrcstkrr
 * // Do some other processing here that you don't show...
c     orhkey        reade     drcst1
c                   enddo