14
\$\begingroup\$

I recently started Learning COBOL from Lynda and this is my code for the first challenge .

The Objective is that the user will provide the radius and we have to calculate the Circumference and the area of the circle and then display it.

Here is the Code.

       IDENTIFICATION DIVISION.
   PROGRAM-ID. CIRCUMFERENCE AS "CIRCUMFERENCE".
   AUTHOR. ASWIN MOHAN.

  ***************************************************************
  *    Create a Program to read the Radius of a circle and THEN
  *    print out the Circumference and the radius of the circle.
  ***************************************************************

   ENVIRONMENT DIVISION.

   CONFIGURATION SECTION.
   SOURCE-COMPUTER.
   OBJECT-COMPUTER.

   INPUT-OUTPUT SECTION.

   DATA DIVISION.

   FILE SECTION.

   WORKING-STORAGE SECTION.
    01   WS-CONSTANTS.
       05    WS-PI                 PIC 9V9999  VALUE 3.1415.
    01   WS-CIRCLE.
       05    WS-RADIUS             PIC 999V99 VALUE ZEROES.
       05    WS-CIRCUMFERENCE      PIC 9999V99 VALUE ZEROES.
       05    WS-AREA               PIC 99999V99 VALUE ZEROES.
    01   WS-DISPLAY.
       05    WS-DISPLAY-CIRCUMFERECE   PIC ZZ99.99.
       05    WS-DISPLAY-AREA           PIC ZZ999.99.


   PROCEDURE DIVISION.
    0100-PROCESS-RECORDS.

        DISPLAY "Enter the Radius".
        ACCEPT WS-RADIUS.

        COMPUTE WS-CIRCUMFERENCE = 2 * WS-PI * WS-RADIUS.
        MOVE WS-CIRCUMFERENCE TO WS-DISPLAY-CIRCUMFERECE.

        COMPUTE WS-AREA = WS-PI * WS-RADIUS * WS-RADIUS.
        MOVE WS-AREA TO WS-DISPLAY-AREA.

        DISPLAY "The Circumferece is " , WS-DISPLAY-CIRCUMFERECE.
        DISPLAY "The Area is " , WS-DISPLAY-AREA.

        STOP RUN.

        END PROGRAM CIRCUMFERENCE.

I'm using GNU-COBOL and I'm worried that my COBOL coding style is not so great. I'm just starting out so all feedback is appreciated.

\$\endgroup\$
  • 2
    \$\begingroup\$ There's a bug in the compiler which is exposed by your code. It will be fixed real soon. See the end of my answer for a work-around if you are not building the compiler from a download of up-to-date sources. \$\endgroup\$ – Bill Woodger May 21 '16 at 20:52
10
\$\begingroup\$

For the working Storage, I would suggest 2 changes

  • align the picture clauses
  • align the 05 directly under name on the 01 tag e.g.
01  WS-CONSTANTS.
    05 WS-PI                     PIC 9V9999  VALUE 3.1415.
01  WS-CIRCLE.
    05 WS-RADIUS                 PIC 999V99 VALUE ZEROES.
    05 WS-CIRCUMFERENCE          PIC 9999V99 VALUE ZEROES.
    05 WS-AREA                   PIC 99999V99 VALUE ZEROES.
01  WS-DISPLAY.
    05 WS-DISPLAY-CIRCUMFERECE   PIC ZZ99.99.
    05 WS-DISPLAY-AREA           PIC ZZ999.99.

You could also align the picture clause (I would not bother though):

 01  WS-CIRCLE.
     05 WS-RADIUS                 PIC   999V99 VALUE ZEROES.
     05 WS-CIRCUMFERENCE          PIC  9999V99 VALUE ZEROES.
     05 WS-AREA                   PIC 99999V99 VALUE ZEROES.

For the procedure division,

  • only use '.' when you absolutely need to
  • personally I would add an exit procedures
   0000-PROCESS-RECORDS.

        DISPLAY "Enter the Radius"
        ACCEPT WS-RADIUS

        COMPUTE WS-CIRCUMFERENCE = 2 * WS-PI * WS-RADIUS
        MOVE WS-CIRCUMFERENCE TO WS-DISPLAY-CIRCUMFERECE

        COMPUTE WS-AREA = WS-PI * WS-RADIUS * WS-RADIUS
        MOVE WS-AREA TO WS-DISPLAY-AREA

        DISPLAY "The Circumferece is " , WS-DISPLAY-CIRCUMFERECE
        DISPLAY "The Area is " , WS-DISPLAY-AREA

        .
   0000-exit.

        STOP RUN.

be careful with your procedure numbering, they typically have specific meaning. There are several different styles, but generally they will give you the call structure In a typical numbering style would be procedure 2310- will be called from procedure 2300- which is intern called from procedure 2000. Normally 2320- is called

A typically for a batch program it might be

  0000-Process.

       Perform 1000-Initialise
       Perform 2000-Process-Records
         until no-more-records
       Perform 3000-Finalise
       .
  0000-X-Exit.      
       Exit.

  1000-Initialise.

       perform 1100-Initialse-Files
       perform 1200-Initialse-DB-Fields
       perform 1300-Prime-Cursore
       perform 9100-Read-From-Cursor

       ...

 2000-Process-Records.

       perform 2100-Match...
       perfrom 2200-...
       ...

2100-Match..

      perform 2110- ....
\$\endgroup\$
12
\$\begingroup\$

Is Lynda that woman from LinkedIn, or just some mate of yours? I wonder how good a teacher she is, and what her intent is. Is it to learn COBOL, or GnuCOBOL.

The reason I say that is that COBOL has Standards. The latest is 2014, which supersedes 2002, then there's 1985 and 1974. There is a 1968 Standard, but it is extremely unlikely that any current compiler in use is to that Standard.

There are current compilers to the 1974 Standard. However, I'd not recommend that as a lowest common denominator, because the 1985 Standard introduced so much that eases coding and use of COBOL.

If learning COBOL (or being taught COBOL) with the intention of using it in a paid job, I'd recommend sticking to the 1985 Standard.

There are also Language Extensions. Everyone has them. Sometimes they coincided, sometimes they don't. GnuCOBOL supports many Extensions from other compilers. However, again, if the aim is to learn COBOL to get a job, beware of the casual use of Extensions.

An area where there are an absolute plethora of Extensions (to 1985) is in human interaction. DISPLAY and ACCEPT can be entirely different beasts from compiler to compiler. To use them in a portable way, use their very simplest forms.

This is sometimes not easy, because although your PROCEDURE DIVISION use is simple, you are, unknowingly, using a GnuCOBOL extension which provides conversion when the target of ACCEPT is a numeric value. If you were to work on an IBM COBOL, the same code would not be reliable.

I agree with Bruce and Edward in their answers. Do something about how your program looks. Exactly what you do in a personal sense depends. Be aware that if you get a COBOL job, you will likely be working in a team, and there will likely be local standards for how your program looks, and how things are done. This eases the ability to pick up someone else's program. You will be able to suggest changes for "bad" practices, but if you can't win the argument, do what the others do at your site.

Next, not so much a COBOL thing as a "programming" thing, I hope, although there are COBOL specifics affecting it. Your program doesn't work.

You can take from the user a maximum value of 999.99 for a radius. Your definitions, specifically for the area, do not allow for a radius of that size.

In COBOL you have to think longer about such things, because definitions, except when they are not, are fixed in size. The size you have for your fields containing "area" are orders of magnitude off. The maximum area is over three million for your maximum radius, but you only allow for five integer digits in your definitions.

Edward suggests using a more accurate value for PI. This is not something to be done blindly. The precision of PI to use should be defined in the specification for the program, as should any and all other limits and required precision. You can't write a COBOL program and then say to the Accountant "well, it works the way I've defined it".

Readability of output is also important. Use separators for large numbers, and zero-suppress down to the single digit before the decimal point. Unless your specification says otherwise. Line the numbers in the output on the decimal point. Much easier to consume as a human reader. Or switch the output to columnar with headings. Depends on the spec.

Be careful of using VALUE clauses unnecessarily. All of your VALUE ZEROES (which I'd code as "VALUE ZERO", but what the heck) are unnecessary. All of those fields are targets before they are sources, so the value of zero will never be relevant to anything. Having the VALUE implies that the value has a significance, which is misleading when it does not. Don't worry, very few programmers actually do that :-)

Indentation is for meaning. If the indentation makes the meaning clearer, do it. If not, don't. There is no point that I can discern in indenting 01-levels beyond what is necessary, or paragraph/SECTION-names.

No one mentioned the commas. Don't use commas (except in literals). They have no meaning. Don't use semi-colons. They have no meaning. To confirm this, add this line anywhere in the PROCEDURE DIVISION and recompile.

        ,,,,;;; , , , ; ; ;

Still compile as before? Still run as before? What is the point then. Ok, how do you make it easier for a human to read when there are multiple targets/sources:

        ADD A
            B
            C                    TO D
                                    E 
                                    F

The names are just for examples, and you should always use meaningful names, for everything. It helps the source code tell the story of the program.

If you are going to use comments, make sure they are accurate. Comments are never compiled, so are only ever "checked" by the human reader. Your comments have significant errors and misinformation. Don't do that. If you then find there is little to say in a comment, because you have structured your program, and used good names, feel free not to use many comments (subject to local standards, of course, which can at times be thoroughly stupid in this particular area).

Here's your program applying most of the above. I've left the ACCEPT changes as an exercise for you.

   IDENTIFICATION DIVISION.
   PROGRAM-ID. CIRCUMF.
  *    Obtain the Radius of a circle and THEN
  *    print out the Circumference and the area of the circle.
   DATA DIVISION.
   WORKING-STORAGE SECTION.
   01  WS-PI                               PIC 9V9999  
                                            VALUE 3.1415.
   01  WS-CIRCLE.
       05  WS-RADIUS                       PIC 999V99.
       05  WS-CIRCUMFERENCE                PIC 9(4)V99.
       05  WS-AREA                         PIC 9(7)V99.
   01  WS-DISPLAY.
       05  WS-DISPLAY-CIRCUMFERECE         PIC Z,ZZZ,ZZ9.99.
       05  WS-DISPLAY-AREA                 PIC Z,ZZZ,ZZ9.99.
   PROCEDURE DIVISION.
       DISPLAY 
               "Enter the Radius"
       ACCEPT 
              WS-RADIUS
       COMPUTE WS-CIRCUMFERENCE     = 2 * WS-PI 
                                      * WS-RADIUS 
       MOVE WS-CIRCUMFERENCE        TO WS-DISPLAY-CIRCUMFERECE
       COMPUTE WS-AREA              = ( WS-PI 
                                        * ( WS-RADIUS 
                                          * WS-RADIUS ) )
       MOVE WS-AREA                 TO WS-DISPLAY-AREA
       DISPLAY 
               "The Circumferece is "  
               WS-DISPLAY-CIRCUMFERECE
       DISPLAY 
               "The Area is         " 
               WS-DISPLAY-AREA
       STOP RUN
       .

For the ACCEPT, you should first DISPLAY to tell the user what they are doing, and the limits to what they enter. The target of the ACCEPT should be an alpha-numeric (PIC X field of some size) and you should process, including validation, what is entered and only treat it as a number within the permissible range if that is confirmed. Try running your program with 999999999.999999 as input, and ponder.

In looking at this, I encountered something curious, and have posted as a possible issue on the GnuCOBOL site at SourceForge.Net. On the ACCEPT, for any input value above 99.99 I get a zero as the second decimal place, even if I have entered a non-zero digit there.

Confirmed as a bug. The fix is in shortly.

I you are building your compiler from a package, or installing a pre-built one, a work-around for the issue:

       05  WS-RADIUS-OVERSIZE              PIC 9(4)V99.
       05  FILLER 
           REDEFINES WS-RADIUS-OVERSIZE.
           10  FILLER                      PIC X.
           10  WS-RADIUS                   PIC 999V99.

And

       ACCEPT WS-RADIUS-OVERSIZE

Instead of the existing ACCEPT, and then use WS-RADIUS as previously.

The code will then continue to work even with the new compiler when updated.

You should clearly comment any code like that, that is a work-around for some issue or other.

\$\endgroup\$
  • \$\begingroup\$ Never Thought COBOL was such a beast under the hood. Always thought it would be just another programming Language. Got to take it more serious Now. \$\endgroup\$ – Aswin Mohan May 22 '16 at 16:01
  • 2
    \$\begingroup\$ Under the hood is a roaring locomotive, well suited to what COBOL was designed to do, process lots of data, fast. These days you can have your own compiler on your own machine, and interface it to what you like (visit the GnuCOBOL site for examples). For a COBOL job, you need to be aware that despite the Standards, not all COBOLs do things the same way. There's extensive documentation available, you should get the GnuCOBOL 2.0 Programmers Guide, and you can get the Enterprise COBOL Language Reference and Programming Guide from the IBM website. \$\endgroup\$ – Bill Woodger May 22 '16 at 18:25
  • \$\begingroup\$ ...And you shall adapt yourself to the local's habits. Many things are forbidden at compilation, and the list varies greatly from site to site. \$\endgroup\$ – gazzz0x2z Jun 2 '16 at 13:45
7
\$\begingroup\$

This is a very good first program. I have only four recommendations:

  • follow Bruce Martin's suggestions;
  • delete the following useless code: AS "CIRCUMFERENCE", the entire ENVIRONMENT DIVISION and the empty FILE SECTION;
  • use the more accurate FUNCTION PI instead of WS-PI; and
  • delete the AUTHOR paragraph: it's been obsolete since 1985 and it's no better than a comment.

Also, in future, use this syntax to define constants:

01  my-constant CONSTANT 10000000.

This has the benefit that the compiler checks that you do not accidently write a value to your constant.

\$\endgroup\$

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.