From: mstemper@ems.cdc.com (Michael Stemper)
Newsgroups: comp.lang.fortran
Subject: Re: [HELP] what's wrong with the following simple code?
Date: 29 Sep 92 18:05:30 GMT
Reply-To: mstemper@ems.cdc.com
Organization: Empros Systems International, a division of Ceridian

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:

|>           subroutine test 
|>            common c, isize,irc,sw,refHz,refpt,sf,iax,ph0,ph1

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.

|>
|>            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?

You can't generally read from a new file

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.

|>       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.

|>              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

|>              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)?

|>            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.

If 'R' and 'r' are actually the same variable, you should have just
read directly 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.

You just overwrote what you previously read into R. 

|>            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.

-- 
#include <Standard_Disclaimer.h>
Michael F. Stemper
Power Systems Consultant
mstemper@ems.cdc.com
Newsgroups: comp.lang.fortran