[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: [OT] C programming, variable size array



<nitpick>

>>>>> "John" == John Smith <netman1@home.nl> writes:

John> #include <stdio.h>
John> int main
John> (
John>    int  nNumberofArguments,

Your variable names are too long, which decreases readability.  Having
such a long name does not convey any more information than a shorter
name such as "numArgs" (or "numArguments").  Not to mention that
Hungarian notation is ugly.

Also, since your program does not make use of the command line
arguments, you might as well just omit the parameters to main.

John>    char* apszArgument []
John> )
John> {
John>    int nReturncode = 0 ;
John>    int* pnStorage = NULL ;
John>    int* pnTmp = NULL ;
John>    int nNumberofelements = 0 ;
John>    int nNumberofreadfields = 0 ;

This variable name is misleading, since its name implies that it holds
some sort of a count, rather than the user input.

John>    int nCounter = 0 ;

Variables should only be introduced in the contexts in which they are
needed.  (Same with pnTmp)

John>    int nInput = 0 ;
 
John>    while ((nReturncode == 0) && ((nNumberofreadfields = scanf("%d",
John> &nInput)) == 1))
John>       {
John>          nNumberofelements++ ;
John>          if (pnStorage != NULL)
John>             pnTmp = pnStorage ;
John>          if ((pnStorage = (int *) malloc (sizeof (int) *
John> nNumberofelements)) != NULL)

Better to deal with the error condition first.  It is much shorter, and
much more straightforward.

if ((pnStorage = ... ) == NULL)
{
  fprintf(...);
  exit(1);
}

...

Also, you should be using realloc to do reallocating, instead of
manually copying every element.

John>             {
John>                nCounter = 0 ;
John>                if (pnStorage != NULL)
John>                   for (nCounter = 0 ; nCounter < (nNumberofelements - 1)
John> ; nCounter++)
John>                      pnStorage[nCounter] = pnTmp[nCounter] ;
John>                pnStorage[nCounter] = nInput ;
John>                free (pnTmp) ;
John>             }
John>            else
John>             {
John>                fprintf (stderr, "Not enough memory\n") ;
John>                nReturncode = 1 ;

Better to exit immediately, using "exit(1)".  Setting the return code,
and then checking it in your while condition makes the while condition
much harder to read.  Using exit also eliminates the need for another if
statement below.

"Returncode" is also a completely non-descriptive name.  You should
choose something better.

John>             }
John>       }
John>    if (nReturncode == 0)
John>       {
John>          for ( nCounter = 0 ; nCounter < nNumberofelements ; nCounter +=
John> 1 )
John>             fprintf (stdout,"pnStorage [%d] : %d\n", nCounter, pnStorage
John> [nCounter]) ;
John>          fprintf (stdout,  "=====================\nNumber of elements :
John> %d\n", nNumberofelements) ;
John>       }
John>    free (pnStorage) ;
 
John>    return nReturncode ;
John> }

-- 
Hubert Chan <hubert@uhoreg.ca> - http://www.uhoreg.ca/
PGP/GnuPG key: 1024D/124B61FA
Fingerprint: 96C5 012F 5F74 A5F7 1FF7  5291 AF29 C719 124B 61FA
Key available at wwwkeys.pgp.net.   Encrypted e-mail preferred.



Reply to: