Following is an extract from an existing program. Though plenty of comments, it is impossible to see the wood from the trees. It takes 124 records, or an equivalent of more than four 3270 screens, to process input cards based on the keyword they contain.
/* Initialization Section */
proindex = 1 /* Initialize index to 1 */
DO FOREVER
/* More recs to process? */
IF (proindex <= input_recs.0) THEN
DO
parse upper var input_recs.proindex keyword data
SELECT
/* process CANCEL cntl rec */
WHEN (keyword = 'CANCEL') THEN
DO
/* currently processing a user */
IF (user_name ^= "") THEN
call procpei00 /* end previous group */
call procpca00 keyword data /* process CANCEL */
END
/* process USERNAME rec */
WHEN (keyword = 'USERNAME') THEN
DO
/* currently processing a user */
IF (user_name ^= "") THEN
call procpei00 /* end previous group */
call procpin00 keyword data /* process USERNAME */
END
/* process RACF cntl rec */
WHEN (keyword = 'RACF') THEN
/* currently processing a user */
IF (user_name ^= "") THEN
call procpal00 keyword data /* process RACF */
ELSE
DO
/* Warning: Record is not associated with a user */
/* It will be ignored. */
call procpro06 /* track warning?? */
END
/* process NOTIFY cntl rec */
WHEN (keyword = 'NOTIFY') THEN
/* currently processing a user */
IF (user_name ^= "") THEN
call procpno00 keyword data /* process NOTIFY */
ELSE
DO
/* Warning: Record is not associated with a user */
/* It will be ignored. */
call procpro06 /* track warning?? */
END
/* process MINIDISK cntl rec */
WHEN (keyword = 'MINIDISK') THEN
/* currently processing a user */
IF (user_name ^= "") THEN
call procppr00 keyword data /* process MINIDISK */
ELSE
DO
/* Warning: Control record is not associated with a */
/* group. It will be ignored. */
call procpro06 /* track warning?? */
END
/* process USERID cntl rec */
WHEN (keyword = 'USERID') THEN
/* currently processing a user */
IF (user_name ^= "") THEN
call procprl00 keyword data /* process USERID */
ELSE
DO
/* Warning: Control record is not associated with a */
/* group. It will be ignored. */
call procpro06 /* track warning?? */
END
/* process RESULTS cntl rec */
WHEN (keyword = 'RESULTS') THEN
/* currently processing a user */
IF (user_name ^= "") THEN
call procprs00 keyword data /* process RESULTS */
ELSE
DO
/* Warning: Control record is not associated with a */
/* group. It will be ignored. */
call procpro06 /* track warning?? */
END
/* process LINK cntl rec */
WHEN (keyword = 'LINK') THEN
/* currently processing a user */
IF (user_name ^= "") THEN
call procpsy00 keyword data /* process LINK */
ELSE
DO
/* Warning: Control record is not associated with a */
/* group. It will be ignored. */
call procpro06 /* track warning?? */
END
/* process TIME cntl rec */
WHEN (keyword = 'TIME') THEN
/* currently processing a user */
IF (user_name ^= "") THEN
call procpti00 keyword data /* process TIME */
ELSE
DO
/* Warning: Control record is not associated with a */
/* group. It will be ignored. */
call procpro06 /* track warning?? */
END
/* comment or blank rec*/
WHEN (LEFT(keyword,1)='*' ! keyword = '') THEN
DO
/* Skip over blank and comment records */
END
OTHERWISE
/* Warning: Invalid record encounterd. */
call SendMsg('1238 keyword')
warningflag = 1
END
proindex = proindex + 1 /* increment index by 1 */
/* CONTINUE LOOP: PROCPRO02 */
END
ELSE
DO
/* end of loop */
LEAVE /* EXIT LOOP: PROCPRO02 */
END
END /* END LOOP: PROCPRO02 */
/* Termination Section */
call procpro03 /* clenaup things */
return /* */
Below you can see the result after applying some of our coding styles, especially those that produce compacter code. We were able to reduce the size to 30 lines, hence this fits on one 3270 screen. We have combined the THEN's and the DO's on one record, put the comments on the same line when possible and removed trivial comments. We have eliminated DO-END constructs when only one instruction must be executed. Instead of performing a test for the presence of a USER card in each of the WHEN clauses, we replaced this by one extra WHEN, with the consequence that the IF-THEN-ELSE constructs could be eliminated also.
We could further enhance the code and for example choose more explicit names for the subroutines, such as Process_Notify instead of proppno00 to indicate we further process the NOTIFY card. Anyway, with the optimization applied here, even a beginning REXX programmer will be able to understand the logic easily.
/* Initialization Section */
DO proindex=1 to input_recs.0 /* Process all records */
/* We will reject most records if no USERNAME record has been */
/* found yet. This is the case when "User_name='' " */
parse upper var input_recs.proindex keyword data
SELECT
WHEN (keyword = 'CANCEL') THEN DO
IF (user_name ^= "") THEN call procpei00 /* end previous user */
call procpca00 keyword data /* process CANCEL */
END
WHEN (keyword = 'USERNAME') THEN DO
IF (user_name ^= "") THEN call procpei00 /* end previous user */
call procpin00 keyword data /* process USERNAME */
END
WHEN user_name = '' then call procpro06 /* Warning: no user yet */
WHEN (keyword = 'RACF') THEN call procpal00 keyword data
WHEN (keyword = 'NOTIFY') THEN call procpno00 keyword data
WHEN (keyword = 'MINIDISK') THEN call procppr00 keyword data
WHEN (keyword = 'USERID') THEN call procprl00 keyword data
WHEN (keyword = 'RESULTS') THEN call procprs00 keyword data
WHEN (keyword = 'LINK') THEN call procpsy00 keyword data
WHEN (keyword = 'TIME') THEN call procpti00 keyword data
WHEN (LEFT(keyword,1)='*' ! keyword = '') THEN nop /* cmt or blank */
OTHERWISE /* Warning: Invalid record encounterd. */
call SendMsg('1238 keyword')
warningflag = 1
END /* Select */
END proindex /* End Process all records */
call procpro03 /* Terminate and cleanup */
RETURN
One of the frequently promoted coding rules is to have only one exit point in a program or subroutine. In general, we can agree with the single exit point of the program, but when applying this rule to the exit point (hence return point) of a subroutine, the code can become needlessly lengthy. Look at this first example:
/* FUNCTION: Validate a fn, ft, userid, or nodeid */
Validate: /* */
/* Initialization Section */
parse arg word /* */
retc = 0 /* initialize return code */
SELECT
/* length too long */
WHEN (LENGTH(word) > 8) THEN
retc = 99 /* invalid parameter */
/* was name an asterisk? */
WHEN (word = '*') THEN
retc = 99 /* invalid parameter */
OTHERWISE
'VALIDATE ' word '* *' /* validate the parameter */
retc = rc /* save the return code */
END
RETURN retc
Let's apply some cosmetics. One is to return from the subroutine as soon as a condition is found that requires no further processing in the subroutine.
/* FUNCTION: Validate a fn, ft, userid, or nodeid */
Validate:
parse arg word
SELECT
WHEN LENGTH(word)>8 THEN return 99 /* Too long */
WHEN word = '*' THEN return 99 /* No wildcards allowed */
OTHERWISE
'VALIDATE ' word '* *' /* Use CMS VALIDATE to check */
return rc
END
Another example shows a piece of code where that same coding rule leads to such a flood of nested IF-THE-DO-END constructs that there is a risk to overflow the right margin if each indentation step would be more than the one character used here.
Honestly, can you follow the logic ? We can't !
/* Need to save CMS? */
IF (do_we_save_CMS = 'YES') THEN
DO
/* Initialization Section */
CALL PROCST784 /* Be sure CMSSAVER is up */
/* CMSSAVER is now up? */
IF (exitrc = 0) THEN
DO
CALL PROCESI00 'IPL 190 PARM SAVESYS' cms_name
/* SEND successful? */
IF (si_rc = 0) THEN
DO
CALL PROCEXA00 'SYSTEM SAVED,HCPNSS440I'
CALL PROCEVM00 chk_response
SELECT
/* Proper response? */
WHEN (vm_rc = 0) THEN
DO
CALL PROCEWV00 /* Wait for VM READ */
SELECT
/* VM READ? */
WHEN (wv_rc = 0) THEN
DO
IF (cplevel = 'VM/ESA') THEN
DO
CALL PROCESI00 'SET MACHINE XC'
/* SEND successful? */
IF (si_rc = 0) THEN
DO
CALL PROCEWA00 /* Wait for any READ */
SELECT
/* any READ? */
WHEN (wa_rc = 0) THEN
DO
CALL PROCESI00 'IPL 190 PARM SAVESYS' cms_name
/* SEND successful? */
IF (si_rc = 0) THEN
DO
CALL PROCEXA00 'SYSTEM SAVED,HCPNSS440I'
CALL PROCEVM00 chk_response /* Check response */
SELECT
/* Proper response? */
WHEN (vm_rc = 0) THEN
DO
CALL PROCEWV00 /* Wait for VM READ */
/* VM READ? */
IF (^(wv_rc = 0)) THEN
/* terminal interrupt */
IF (wv_rc = 8) THEN
exitrc = 6
ELSE
exitrc = 8
END
/* terminal interrupt */
WHEN (vm_rc = 8) THEN
exitrc = 6
OTHERWISE
exitrc = 8
END
END
ELSE
exitrc = 4
END
/* terminal interrupt */
WHEN (wa_rc = 8) THEN
exitrc = 6
OTHERWISE
exitrc = 8
END
END
ELSE
exitrc = 4
/* Termination Section */
/* END PROCEDURE: PROCST646 */
END
END
/* terminal interrupt */
WHEN (wv_rc = 8) THEN
exitrc = 6
OTHERWISE
exitrc = 8
END
END
/* terminal interrupt */
WHEN (vm_rc = 8) THEN
exitrc = 6
OTHERWISE
exitrc = 8
END
END
ELSE
exitrc = 4
END
.......
END
This same code can be dramatically reduced in size and gain in readability by using a new subroutine SaveCMS that returns immediately as soon as an error is detected.
IF (do_we_save_CMS = 'YES') THEN DO /* Need to save CMS ? */
SaveRc=SaveCms()
... postprocessing ...
if SaveRc=0 then ...
else ...
end
.....
/*---------------------------------------------------------------------*/
SaveCms: /* */
/*---------------------------------------------------------------------*/
CALL PROCST784 /* Be sure CMSSAVER is up */
IF (exitrc <> 0) THEN return exitRc /* CMSSAVER is down */
CALL PROCESI00 'IPL 190 PARM SAVESYS' cms_name
IF (si_rc <> 0) THEN return si_rc /* SEND failed */
CALL PROCEXA00 'SYSTEM SAVED,HCPNSS440I'
CALL PROCEVM00 chk_response
if (vm_rc <> 0) THEN return 8 /* unexpected response */
CALL PROCEWV00 /* Wait for VM READ */
IF (wv_rc =8 ) THEN return 6 /* unexpected terminal interrupt*/
IF (wv_rc <> 0) THEN return 8 /* timeout waiting for VMREAD */
IF (cplevel = 'VM/ESA') THEN DO
CALL PROCESI00 'SET MACHINE XC'
IF (si_rc <> 0) THEN return si_rc /* SEND failed */
CALL PROCEWA00 /* Wait for any READ */
if (wa_rc <> 0) THEN return 8 /* any READ? */
CALL PROCESI00 'IPL 190 PARM SAVESYS' cms_name
IF (si_rc <> 0) THEN return si_rc /* SEND failed */
CALL PROCEXA00 'SYSTEM SAVED,HCPNSS440I'
CALL PROCEVM00 chk_response /* Check response */
if (vm_rc <> 0) THEN return 8 /* unexpected response */
CALL PROCEWV00 /* Wait for VM READ */
IF (wv_rc =8 ) THEN return 6 /* unexpected terminal interrupt*/
IF (wv_rc <> 0) THEN return 8 /* timeout waiting for VMREAD */
END
RETURN 0
We hope these few examples have convinced you.
Use the backward navigation button of your browser to return to the text.