6
\$\begingroup\$

I was teaching some new people in office a bit of COBOL, so I threw together this exercise where they had to write a program that would generate the first 10 numbers of a Fibonacci suite and then test them against the rules of .

I was hoping to get an honest review of this code to see how I could improve.

   IDENTIFICATION DIVISION.
   PROGRAM-ID. PROG6.
   AUTHOR.        SAGGING RUFUS.
   DATE-WRITTEN.  JUNE 5 2017.
   DATE-COMPILED.
   TITLE "FIZZBUZZONACCI"

   ENVIRONMENT DIVISION.
   CONFIGURATION SECTION.
   SOURCE-COMPUTER.   AMDAHL-580.
   OBJECT-COMPUTER.   AMDAHL-580.

   DATA DIVISION.
   WORKING-STORAGE SECTION.
   01  WS-STORAGE.
       05 WS-SUB PIC 9(8) COMP.
       05 WS-LIMIT PIC 9(4) VALUE 10.
       05 WS-F.
          10 WS-FX PIC 9(8) COMP.
          10 WS-F1 PIC 9(8) COMP VALUE 0.
          10 WS-F2 PIC 9(8) COMP VALUE 1.
       05 WS-F-ARRAY      PIC 9(8) COMP OCCURS 10 TIMES.
       05 WS-F-SUB        PIC 9(8) COMP VALUE 0.

   01  COUNTERS.
       05 CURRENT-NUM PIC 9(3) COMP.

   01  REMAINDERS.
       05 MOD-THREE PIC 99 COMP.
          88 FIZZ VALUE 0.
       05 MOD-FIVE PIC 99 COMP.
          88 BUZZ VALUE 0.

   PROCEDURE DIVISION.

   0000-MAINLINE.
       PERFORM VARYING WS-SUB
                  FROM 1 BY 1
                 UNTIL WS-SUB > WS-LIMIT
          COMPUTE
             WS-FX = WS-F1 + WS-F2
          END-COMPUTE

          ADD 1 TO WS-F-SUB

          MOVE WS-F2 TO WS-F1
          MOVE WS-FX TO WS-F2
          MOVE WS-FX TO WS-F-ARRAY(WS-F-SUB)
       END-PERFORM

       PERFORM VARYING WS-SUB
                  FROM 1 BY 1
                 UNTIL WS-SUB > WS-LIMIT
          MOVE WS-F-ARRAY(WS-SUB) TO CURRENT-NUM

          COMPUTE
             MOD-THREE = FUNCTION MOD(CURRENT-NUM 3)
          END-COMPUTE

          COMPUTE
             MOD-FIVE = FUNCTION MOD (CURRENT-NUM 5)
          END-COMPUTE

          EVALUATE TRUE
             WHEN FIZZ AND BUZZ
                DISPLAY "FIZZBUZZ"
             WHEN FIZZ
                DISPLAY "FIZZ"
             WHEN BUZZ
                DISPLAY "BUZZ"
             WHEN OTHER
                DISPLAY CURRENT-NUM
          END-EVALUATE
       END-PERFORM

       STOP RUN.
\$\endgroup\$
  • \$\begingroup\$ Should make ws-limit comp since you compare it to ws-sub which is comp \$\endgroup\$ – Bruce Martin Aug 16 '17 at 21:46
  • \$\begingroup\$ @BruceMartin Comments are for seeking clarification to the question, and may be deleted. Please put all suggestions for improvements in answers, even if they are short, simple observations. \$\endgroup\$ – 200_success Aug 17 '17 at 5:45
4
\$\begingroup\$

My suggestions focus on style issues and pointless code and assume you're using a modern compiler.

In the identification division and environment division:

  • Delete the comment paragraphs. They were deleted from the standard 15 years ago and have been obsoleted by version control systems.
  • Rather than use the TITLE statement, name your program FIZZBUZZONACCI. All contemporary compilers support program-names longer than six characters.
  • Delete the non-functional SOURCE- and OBJECT-COMPUTER paragraphs.

In the data division:

  • WS-LIMIT is a constant, so it's good idea to declare it as such. Change its declaration to

    01  WS-LIMIT CONSTANT AS 10.
    

    or, if your compiler doesn't support that, try the common non-standard extension:

    78  WS-LIMIT VALUE 10.
    
  • Some of the variables begin with WS- and some don't. I suggest getting rid of the WS- prefix: there's nothing to gain here from indicating a variable is in working-storage.
  • Similarly, there's nothing to gain from putting variables in the WS-STORAGE, COUNTERS and REMAINDERS group (except for the OCCURS items).

In the procedure division:

  • Get rid of the redundant END-COMPUTE's. They're only needed for the SIZE ERROR handlers and clutter the code otherwise.

Quuxplusone raises a good point that FIZZ and BUZZ aren't strictly necessary. You could rewrite the EVALUATE like so, and not hide the meaning of FIZZ and BUZZ in the data division:

EVALUATE mod-three ALSO mod-five
    WHEN 0 ALSO 0
        DISPLAY "FizzBuzz"
    WHEN 0 ALSO ANY
        DISPLAY "Fizz"
    WHEN ANY ALSO 0
        DISPLAY "Buzz"
    WHEN OTHER
        DISPLAY current-num
END-EVALUATE
\$\endgroup\$
  • \$\begingroup\$ Re FIZZ vs MOD-THREE: Does COBOL have any way to write the equivalent of fizz = ((current_num % 5) == 0) or fizz = (mod_three == 0)as a computation? That would IMHO be the clearest way to write it. \$\endgroup\$ – Quuxplusone Aug 16 '17 at 22:18
  • \$\begingroup\$ @Quuxplusone No, the closest thing is the 88-level items he's already using. \$\endgroup\$ – Edward H Aug 17 '17 at 9:09
  • 1
    \$\begingroup\$ I agree with everything you said about the data division and the procedure division. The only reason I added the information in the environment division is because that is still standard practice at my site, so I wanted to use it to make it similar to our actual code set. I am not sure if our compiler will allow constant, but I will definitely be trying that out! \$\endgroup\$ – SaggingRufus Aug 17 '17 at 10:15
2
\$\begingroup\$

I don't know COBOL, so take this with a large grain of salt.

  • It seems like the values of variables WS-SUB and WS-F-SUB are always equal. Why have two names, then? Just use WS-SUB in both places.

  • I don't understand enough COBOL to know how the computed value gets from MOD-THREE into FIZZ or from MOD-FIVE into BUZZ. But again, why have two different names for the same thing?

  • It seems to me that WS-LIMIT is fundamentally a parameter to this program, whereas WS-SUB, WS-FX, etc. are fundamentally scratch variables. Your data division hides this difference in intent, whereas it should (IMO) be highlighting the difference — at least via a code comment!

  • Your WS-F-ARRAY array is flat-out not needed; you should combine your two loops into a single loop and get rid of the array storage. However, if you really really want to keep the array for pedagogical purposes, at least you should be indexing it by WS-F-SUB in both loops, not WS-F-SUB in the first loop and WF-SUB in the second loop — that's just needlessly confusing. (See above re: having two different names for the same thing.)

  • Seems like your TITLE would make a good PROGRAM-ID, no?

\$\endgroup\$
  • \$\begingroup\$ Very good points. The only thing I cannot do its the recommendation of putting the title as the program ID. I only have 8 characters to work with for the program ID (as per my sites standards). \$\endgroup\$ – SaggingRufus Aug 17 '17 at 10:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.