21
\$\begingroup\$

This is my first COBOL program, and I'd like to get a critique as I am unfamiliar with best practices.

Particularly, I'd like to know how to get the input and output to be more elegant and less dependent on how the file fields are structured.

Some type of analogy to C's scanf(), getch(), or any of the Java console IO routines would be helpful.

https://gist.github.com/anonymous/5427201

        DATA DIVISION.
    WORKING-STORAGE SECTION.
    01 TIME-STORAGE.
        03 CURRENT-TIME-NUMERIC         PIC 9(8).
        03 ETA-NUMERIC PIC 9(8) REDEFINES CURRENT-TIME-NUMERIC.    

        03 CURRENT-TIME REDEFINES CURRENT-TIME-NUMERIC.
            05  CURRENTHOUR     PIC 99.
            05  CURRENTMINUTE   PIC 99.
            05  FILLER          PIC 9(4).
        03 ARIVAL-TIME-NUMERIC PIC 9(8).
        03 ARIVAL-TIME REDEFINES ARIVAL-TIME-NUMERIC.
            05  ARRIVALHOUR     PIC 99.
            05  ARRIVALMINUTE   PIC 99.
            05  FILLER          PIC 9(4).
    PROCEDURE DIVISION.
    BEGIN-BUG-NORMAN.
        DISPLAY "when will you be ariving?"
        DISPLAY "HH [enter]"
        ACCEPT ARRIVALHOUR
        DISPLAY "MM [enter]"
        ACCEPT ARRIVALMINUTE
        ACCEPT CURRENT-TIME FROM TIME.
        SUBTRACT ARIVAL-TIME-NUMERIC 
            FROM CURRENT-TIME-NUMERIC
            GIVING ETA-NUMERIC.
        DISPLAY "RESULTING ETA:"
        DISPLAY ETA-NUMERIC.
        DISPLAY "HOURS:"
        DISPLAY CURRENTHOUR
        DISPLAY "MINUTES:"
        DISPLAY CURRENTMINUTE

        STOP RUN.
\$\endgroup\$
  • 3
    \$\begingroup\$ Might be nice for privileged user to create a COBOL tag. \$\endgroup\$ – Nate Apr 20 '13 at 21:27
  • 2
    \$\begingroup\$ Wow I thought I'd never see actual COBOL code - that's what sent mankind to the moon right? If it's any consolation, I don't think there's a lot of people [alive] familiar with COBOL best-practices :) (no offense intended at any actual COBOL programmer out there!) \$\endgroup\$ – Mathieu Guindon Nov 21 '13 at 21:06
  • 5
    \$\begingroup\$ @retailcoder See exit109.com/~ghealton/y2k/y2k_humor/Cobol.html and weep. \$\endgroup\$ – rolfl Nov 21 '13 at 21:11
  • \$\begingroup\$ @retailcoder: stackoverflow.com/questions/tagged/cobol \$\endgroup\$ – Jamal Nov 21 '13 at 21:33
  • 3
    \$\begingroup\$ I've had to work with COBOL in school last year. I'm still having nightmares \$\endgroup\$ – Jeroen Vannevel Nov 21 '13 at 23:00
19
\$\begingroup\$

A couple of things I noticed:

  1. No Identification Division. "The IDENTIFICATION DIVISION has no effect on the execution of the program but is, nevertheless, required as a means of identifying the program to the computer" - Stern & Stern
  2. There is no real reason for the first REDEFINES. Also, you seem to be using it wrong. A redefines clause is simply another way to reference the same working storage location. If you want to store different data, that will require different variables. While you can keep redefining the same variable and using it in different way, it can get messy and storing things gets harder.
  3. While you can have level 3 variables, I have found it to be best practice to go up in increments of 5
  4. A lot of the time, if you are working in a real COBOL environment, you will need an environment division. Clearly here you don't, but keep that in mind
  5. You should start all variable names in the WORKING-STORAGE SECTION with WS- because if this program need to interact with any other programs, you will have a LINKAGE-SECTION with its own set of variables.
  6. Only use periods to end paragraphs. Technically, you can use a period to end anything. This leads to messy code. Also, a period closes ALL open statements. If you have a triple nested IF and you end with one period, it will close all 3. When you get to that point, use the proper scoop terminators such as END-IF and END-PERFORM
  7. Spacing is important as well.

The logic problems have already been address in another answer, so I will not address that here. I have included the code below with my suggestions added to it:

        INDENTIFICATION DIVISION
        PROGRAM-ID. THE-TIMER.
        AUTHOR. name.
        DATE-WRITTEN. date.
        DATE-COMPILED. date.
        *****************************************************************
        *header comment about the program change logs ect               *
        *****************************************************************

        DATA DIVISION.
        WORKING-STORAGE SECTION.

        01 TIME-STORAGE.
            05 WS-CURRENT-TIME        PIC 9(8).
               10  WS-CURRENTHOUR     PIC 99.
               10  WS-CURRENTMINUTE   PIC 99.
               10  WS-FILLER          PIC 9(4).

            05 WS-ARIVAL-TIME         PIC 9(8).
               10  WS-ARRIVALHOUR     PIC 99.
               10  WS-ARRIVALMINUTE   PIC 99.
               10  WS-FILLER          PIC 9(4).

            05 WS-ETA                 PIC 9(8).
               10 WS-ETAHOUR          PIC 99.
               10 WS-ETAMINUTE        PIC 99.
               10 WS-FILLER           PIC 9(4).


        PROCEDURE DIVISION.

            BEGIN-BUG-NORMAN.

            DISPLAY "when will you be ariving?"
            DISPLAY "HH [enter]"
            ACCEPT WS-ARRIVALHOUR
            DISPLAY "MM [enter]"
            ACCEPT WS-ARRIVALMINUTE

            ACCEPT WS-CURRENT-TIME FROM TIME
            SUBTRACT WS-ARIVAL-TIME 
                FROM WS-CURRENT-TIME
                GIVING WS-ETA

            DISPLAY "RESULTING ETA: "
            DISPLAY ETA-NUMERIC
            DISPLAY "HOURS: "
            DISPLAY CURRENTHOUR
            DISPLAY "MINUTES: "
            DISPLAY CURRENTMINUTE

            IF WS-ETAHOUR > 5
               DISPLAY " It could be a while"
            END-IF

            STOP RUN.
\$\endgroup\$
  • 10
    \$\begingroup\$ Oh, no!!! Someone who actually knows COBOL instead of just can read it! (nice answer, BTW) \$\endgroup\$ – rolfl Jan 3 '14 at 13:39
  • 1
    \$\begingroup\$ Thanks! I am new a new poster to codereview and stackoverflow but a long time reader. I just want to give back! \$\endgroup\$ – SaggingRufus Jan 3 '14 at 13:59
  • \$\begingroup\$ Can anyone tell me why my codeblock wont indent? \$\endgroup\$ – SaggingRufus Jan 3 '14 at 14:01
  • 2
    \$\begingroup\$ I am not sure why the indent on the PROCEDURE DIVISION was wrong (it was wrong in the raw text, not the markup), but, I added additional spaces. I have the code now aligned on column 12. \$\endgroup\$ – rolfl Jan 3 '14 at 14:22
  • 1
    \$\begingroup\$ It could affect the initial compile, but thats it. When you compile, that compiled code is stored in a load library to be run by either a JCL or to be invoked by CICS. I personally add them as do my collegues because readability is dramatically increased. I have also seen people comment out those lines. In my opinion the readability of the code is more important than the inital compile time. \$\endgroup\$ – SaggingRufus Jan 8 '14 at 12:21
10
\$\begingroup\$

While the code is crisp and clean (COBOL always is .... ;-) ) I see some logic flaws in your computations.

Specifically, consider the current time 14450000 (2:45pm) and the user enters 15 for [HH] and 15 for [MM] as his ETA (3:15pm)

Your subtract will store the difference between 14450000 and 15150000. The difference stored in ETA-NUMERIC will be 00700000, and the code will display HOURS:00 and MINUTES:70, but, we know that there's only 30 minutes between them.

The code does not take in to account that minutes are base-60.

\$\endgroup\$
  • \$\begingroup\$ Thanks for pointing out the error in logic. I haven't been able to find an equivalent to jodatime for cobol yet. The company had a bunch of internal date handling code, but it was a bit over my head to try and understand. All the libraries I found that might have worked won't compile with the flags we are forced to use. \$\endgroup\$ – Ape-inago Jan 4 '14 at 14:35
6
\$\begingroup\$

For the most part, you are assuming valid numeric values, nothing will get you a s0c7 faster. In laymans terms, an system completion code 0C7 aka s0c7. It means 'attempting to perform numeric operations on non numeric data.' BTW, if the user enters a space thinking that means zero, bam, s0c7.

So best practices is to add a 'If WS-CURRENTHOUR is numeric then ....'

of course you may want to combine many checks, but that makes meaningful error message more difficult.

\$\endgroup\$
6
\$\begingroup\$

Searching for "Best Practices" is commendable and should benefit you.

However, don't expect agreement, even after 50+ years, of what is and isn't "Best Practice".

That aside, you should have a set of "local standards" for your site. These you should adhere it, as should others there. If they contain dumb things, try to get them discussed, but go with the consensus after that. If everyone has the best chance of understanding the code that everyone else writes, things will be better. Traditionally COBOL systems are team efforts, so understanding other people's code, and making yours understandable, is very important.

Sometimes the local standards are not what many would describe as best practice. They may have been at least good practice at some time in the past, but local standards change slowly.

You ask in a comment about blank lines. Perhaps 40 years ago (less depending on hardware at a particular site) not having blank lines would make sense. Each blank line would be a punched-card. Each card takes some time to read. Each card may become faulty and jam the reader, or be rejected.

Even slightly after that sort of time, CPUs weren't anywhere near as fast as they are today. So reading a blank line just for the compiler to ignore would have some effect on compiler throughput for large programs, and with perhaps one address-space only dedicated to compiles and running 24-hours-a-day, little things add up.

Today, don't worry about it at all. Make your program readable for a human. The compiler will always manage, it is us who needs help in reading programs, so give it to us.

A. MOVE B TO C. PERFORM D. D. MOVE E TO F. PERFORM I. I. MOVE Z TO Q.

That's "valid" COBOL. Theoretically faster to compile than coding eight separate lines (even without blank lines), but work out how much longer it takes a human to understand what is happening.

NEW-CLIENT-RECORD.

    [all the stuff you need to do for establishing a new client record]

    PERFORM                       NEW-CLIENT-SUB-RECORD
    .
NEW-CLIENT-SUB-RECORD.

    [all the stuff you need to do ...]

    PERFORM                       CLIENT-COMPLIANCE-VERIFICATION
    .

CLIENT-COMPLIANCE-VERIFICATION.

    [all the stuff you need to do ...]
    .

Do not try to "save storage" by using REDEFINES. Define separate fields, and avoid the "side effect" headaches, either when you write the code, or when someone (perhaps you) throws in a careless change in two months time.

Use good names for everything. Ditch any idea of VAR1 or I, J, K.

IF VAR1(J)>7
    MOVE "Y" TO VAR4
END-IF

Would you prefer debugging that, or writing this:

IF CT-OF-APPLES-IN-STOCK ( WAREHOUSE-STOCKED-IN )
    GREATER THAN MAX-CT-FOR-PERISHABLES-TYPE-P014 [which has a value of 7]
    SET OVERSTOCKED-ORANGES        TO TRUE
END-IF

In fact, look, you can't even write that about the oranges, can you? Whilst writing, you can even ask yourself (and look at the spec, ask the analyst/designer/business-user) should that be the "count" of the apples, or something like "box" or "pallet" or whatever, because apples in a warehouse aren't usually stored singly.

Try to get anything useful out of the VAR1/I rubbish and you'll spend a lot of time tracking down what a field holds, and what it means.

Make your programs readable. Do nothing to make things obscure. Follow local standards.

Read the manuals. Read other people's code. Experiment. Think about what people suggest, and what benefit it gives you.

For instance, you will find WS01-data-name, WS02-data-name etc. The W is useful, the rest is nonsense which will cause frustration when defining data. Without renaming everything, it will not be possible to include new data in a group of existing data it is related to. You will find local standards dictating such things. Try to get those changed, but go with the flow if you fail (write some macros/scripts to do the work for you, of course).

\$\endgroup\$
  • \$\begingroup\$ I didn't even think of the fact that it may have started out on punch cards. Other than that, it did seem to be a lot of the really old code had to work within the limitations of the time. uo; \$\endgroup\$ – Ape-inago Feb 1 '14 at 19:30
  • \$\begingroup\$ Sadly, even the new development stuff was still being written with those limitations. I ended up burning out on all the COBOL that I had to deal with. But I did get to learn about all sorts of programming anti-patterns. I think I would have enjoyed working on code like your second example. My biggest problem was that there were no real standards to follow and there wasn't any push to make it happen from anyone. I came into it thinking that the stuff you mention should be par for the course in most programming languages, but I don't think that is the case anymore. \$\endgroup\$ – Ape-inago Feb 1 '14 at 19:40
  • 1
    \$\begingroup\$ @Ape-inago I sympathise. It is a language which can be used well, or badly. Many who use it badly, think they use it well. \$\endgroup\$ – Bill Woodger Feb 1 '14 at 22:01

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.