From: sandee@Think.COM (Daan Sandee)
Newsgroups: comp.lang.fortran
Subject: Re: [HELP] what's wrong with the following simple code?
Date: 29 Sep 1992 21:48:37 GMT
Organization: TMC
Distribution: world
NNTP-Posting-Host: telecaster.think.com

In article <27755@nntp_server.ems.cdc.com>, mstemper@ems.cdc.com (Michael Stemper) writes:
|> In article <90835@netnews.upenn.edu>, duong@chestnut.chem.upenn.edu (-3612) writes:
|> |> I'm a fortran novice here. I couldn't figure out what's wrong with the
|> |> following subroutine so could anyone give me a hint? Thank you..
|> 
|> Well, I can find several things wrong with it:

|> Obscure variable names
|> 
|> |>            complex c(65536)
|> |>            real r(131072)
|> |>            integer ip(131072)
|> 
|> Should make 65536 a parameter, make 131072 a second parameter, calculated
|> from it.

Absolutely.
|> 
|> |>
|> |>            equivalence (c,r), (c,ip)        !share data memory
|> |>            open (21, file='ftnmr.dat', form='unformatted',status='new')
|> |>            read (21) i,(R(j),j=1,abs(2*i))
|> 
|> You have defined an array 'r', but nothing called 'R'
|> 
|> Why abs(2*i)? Why not just 2*i?

That will become apparent later ... i (the first word of the input record)
doubles as a flag to tell whether or not this is a data record.
|> 
|> You can't generally read from a new file

Yes, that's a bug.

|> Expecting someone to put the number of data items as the first element
|> in input is bad human factors engineering. The program should read until
|> it encounters EOF or a flag. If a data item count is necessary, the
|> program should count them. Computers count much better than people do.

Unfortunately, in Fortran unformatted input, you have to know the record
length before reading it. Most implementations abort if the record is
shorter than specified, and some abort if the record is longer
than specified. Any read-until-EOF in Fortran is hopelessly inefficient
compared with using unformatted read to read a large record.

On the other hand, there are plenty of implementations that don't allow you
to read a variable, then use it as an implied DO loop boundary in the
same read. These implementations generally use the previous value of i in
the DO loop. In an extremity (when faced with this file structure, which
I didn't create), I have been forced to use
       read (lu) len
       backspace lu
       read (lu) dummy, (data(i),i=1,len)

|> |>       c    interpret 1D parameters
|> |>            if (i.gt.0) then           !this was a data record
|> |>              isize = i                !size, number of data
|> 
|> If you are really going to insist on making the user tell you
|> how many items you have, you should have read directly into
|> isize, not into i and transferring.

i is a temporary variable local to this subroutine, which may or may not
be a record size of significance to the caller. If it is *not* a record
size, then he shouldn't read into the common variable isize. What you are
suggesting introduces a possible bug.

|> |>              irc = 1                  !data are complex by default
|> 
|> I think that you left out an else here? The structure that you shows
|> writing 'no parameters' and returning only when i>0

Sloppy assembly-style programming, nothing more. If the if fails, he
falls through. Big deal.

|> |>              close (21)
|> |>              write (*,'(a)') ' there were no parameters.'
|> |>              return
|> |>            end if
|> |>            isize = float(ip(1))         
|> 
|> Since isize defaults to integer, why are floating ip(1)?

Good question. Especially as he has just read the data into r(..), so
      isize = r(1)
would be the same thing, although still a mystery.

|> |>            irc = ip(2)                  
|> |>            sw = r(17)                   
|> |>            refHz = r(20)                
|> |>            refpt = r(19)                
|> |>            sf = r(18)                   
|> |>            iax = ip(4)                  
|> |>            ph0 = r(22)                  
|> |>            ph1 = r(23)                  
|> 
|> Why 17? Why 22? The above section of code will be impossible to
|> maintain, and probably to use.

It is by now apparent that our friend is not responsible for the 
structure of his data file. Some other program written written by
some other person has created it. So don't blame him for the above.
For the sake of intelligibility, but not necessary maintainability,
he should be using parameters instead of constants. (If this is the
only place where these numbers are used, which I suspect, comments
would be sufficient.)

|> If 'R' and 'r' are actually the same variable, you should have just
|> read directly into refHz, etc.

Some Fortran implementations require a different file format for
    read (lu) a,b,c
as opposed to
    read (lu) x(3)
although in both cases they are reading three real data items.
(This just happened to me when porting a program (to SunOS).)
So, as the file format is predetermined, there is only one way to read it.

Also, this is the 'else' part of the 'if', remember ? So if this is a data
record, he *shouldn't* be reading into refHz, etc.

|> |>       c    read the next record of data
|> |>            read (21, end=2) i,(R(j),j=1,abs(2*i))
|> 
|> Statement 2 is not defined.

Correct.

|> You just overwrote what you previously read into R. 

That appears to have been the intention.

|> |>            close (21)
|> |>            return
|> |>            end
|> 
|> If you are going to play tricks with equivalencing and common, you
|> should have extensive comments. This module doesn't even have enough
|> comments for a straight-forward piece of code, but it is doing some
|> very cryptic things.

To you, maybe. It just requires a bit of a closer look, which you could
have taken before starting to flame.
The data structure (which I presume was fixed by another programmer, or
at least we have data files that already exist) is as follows

 First element, if positive, is the record size. In that case, the rest
  of the input record is data.
 If the first element is negative, the input record contains parameters.
  (Presumably 2*(abs(i)) in that case is at least 23, to cover all
  parameters.) He is not using all 23 input values. Apparently he's not
  interested in all of them.
  The 'record size' to be transmitted to the caller in this case is in ip(1).
  Here we have a mysterious piece of code
          isize = float(ip(1))
  ? If it's a floating-point data item, in the file, that should be
          isize = r(1)    (the compiler will add code for the conversion)
    If it's an integer in the file, it should be
          isize = ip(1)
  The code as found will work, but I don't see the point.
  Finally, if this is a parameter record, then the *next* record contains
  data and is read in. The size of this record is again read out of the
  first word.

With this data structure, I can see no alternative to the strategy he is
using.
I could, of course, supply a lot of constructive criticism.

Bugs :
 - Apparently it hadn't registered yet that the C for the comment line
  should be in column 1. (I assume the whole routine got shifted over,
  which is why the OPEN statement fell off column 72).
 - There is no statement 2 matching the END=2 in the last read.
 - As everybody seems to have discovered, you can't read off a file
   you just opened with status=new.
 - Possibly you will have a problem with reading i off the file than
   using it in the same read statement, as I described above.
Sloppy coding :
 - The IF/THEN should be followed by an ELSE which covers the rest of
   the routine. They are mutually exclusive code sections and should
   be programmed as such. In this manner you will make it more
   intelligible to Fortran-dyslexic persons who otherwise apparently
   may have problems understanding what's going on.
 - There should be lots of comments, of course. This discussion would
   have been much shorter if you had explained what you are doing, instead
   of having me do it.
 - 65536 should be a parameter and 131072 should be a derived parameter.
 - The structure of the data file should be documented in the comments.
   If this is the *only* place in the program where this structure is
   used, you don't have to use parameters for ip(22) or whatever, as that
   is evident from the comments.
 - Why only an end=2 on the last read, not on the first ? Could it be
   that a data record following a parameter record is optional ? If so,
   does isize=ip(1) indicate that ? What were you going to do if it
   had hit the end=2 ?
 - You need to put in some sanity checks. As, what happens when you read
   a record  where i > 65536 ? What if i is zero ? What if -22<2*i<0, i.e.,
   insufficient parameters supplied ? Could this be a good place to
   check on the values of the parameters ? What about the i (record size)
   of the data record read in near the end ? i should be positive, < 65536,
   and probably have some relation to isize=ip(1) in the preceding
   parameter record.
   If you didn't create the data file, don't believe anything they tell you 
   about the format. Some checks added now can save you a lot of hassles.

 That's the bells and whistles department. Also, don't forget the frills
 and tucks department. That is, add comments and have a nice layout. Saves
 you some flames if you ever again post to the net.

Daan Sandee                                           sandee@think.com
Thinking Machines Corporation
Cambridge, Mass 02142                                 (617) 234-5044
Newsgroups: comp.lang.fortran