Title: A Code Review

I fished the following from one of the Internet newsgroups. The writer provided a very substantial chunk of source with the following pre-amble:

/*********************************
*     PRODINV.H
Include file containing program constants, function 
prototypes, and structure declarations for CIS 32A 
class project.
**********************************/

It would have been a good idea to include a sentinel at the beginning of this header file. In a small lab project you will get away without one but good developing good habits from the start will pay dividends later on. So your header file should start with:

#ifndef PRODINV_H
#define PRODINV_H

and end with:

#endif

And his code continues:

#define ADD (1'     /* Menu options */
#define REPORT '2'
#define DELETE '3'
#define CHANGE '4'
#define QUIT '5'

I would strongly advocate that you not (a) use the pre-processor for these and (b) use character codes for the values. I can see no advantage in doing so, particularly as the normal input to a C program is int (which maybe internally converted to a char). I would replace the above with:

enum MenuOptions { add_mo = 1,
                   report_mo,
                   delete_mo,
                   change_mo,
                   quit _mo};

I note that you later provide functions to do each of these things. Unfortunately in C the global namespace is already cluttered by standard library functions. I do not want to use 'delete' as an identifier because it is a C++ keyword. My next choice 'remove' is a Standard C Library function name. In C++ I would use a namespace to prevent possible clashes but I do not have that option in C so I have resorted to using a suffix. I find suffixes less invasive than prefixes and so make reading code easier, what you want to know is at the front and the disambiguator is at the back.

I would use an enum to provide your product type and another for your inventory item limits.

/* Output headings / prompts      */
define HEADING "California Sports Emporium"

<snipped other entries>

define  PDESC_PROMPT " description         : "

You have applied a perfectly conventional solution to isolating your various string literals but this is not efficient and is hard to convert to using an input file to provide alternative headings at execution time. There are various solutions to the problem. Here are a couple for consideration:

Use pointers:

static char const * heading = "California Sports Emporium";

The static qualification is to allow use in multiple translation units without any risk of the linker complaining. The const qualification is because string literals are semantically immutable (and are actually arrays of char const in C++). Just define the other strings in the same way. The problem is that this doesn t easily convert to initialisation from a file. So I prefer the following slightly more complicated mechanism:

char const * literal[ ] = { 
    "California Sports Emporium",
/* rest of items */
    "     description           :",
    "Invalid string" };

with the following enum to make use comprehensible:

enum Literals {heading,
  /* other entries */
    pdesc_prompt,
    end_of_literals };

Now you can write: literal[heading] etc. You will have to work a little harder to ensure that the array of pointers is only initialised once. On the positive side, that initialisation could be quickly converted to a run time one from a file.

define  STRING_SIZE  80
define  MAX_STRING   30

I would prefer to replace those but the only portable (between C and C++) is by using an enum. Pure C++ would use strings (which are expandable) and so would not need these definitions.

              /* Function prototypes   */
void  vfnAdd(double *dpGTPrice,
             double *dpGTCost, FILE *fp);
int   ifnGetInt(char szPrompt[], int iLow,
                int iHigh); /* Gets a legal integer value */
int   ifnGetMore(void);
             /* determines user continue decision */
< rest snipped >

Look carefully at the above. What do you notice? The use of Hungarian notation. Now ask yourself how useful it is? For example, we know vfnAdd is a function and if only we knew what it did it would probably be obvious that it had a void return. Look at those parameter names, how do they help? Yet the writer hasn't provided a comment.

Hungarian notation has added nothing useful and the opportunity to provide a properly descriptive identifier has been missed.

Look at the return type for that third function. Wouldn't some form of boolean type have been better. And why no parameter? I would have expected the prototype of that function to have been something like:

Bool yesNo(char const * prompt);
           /* get y/n response to prompt */

(Assuming that enum Bool {false, true }; has been provided) I am not really happy with that function name but I will live with it for now. I certainly do not find variations on getMore acceptable. That is not what the function does, it gets a response to a question.

/* Structure definition        */
typedef struct STRUCT_INV { 
  int ipnum;
  int iptype;
  char szDesc[MAX_STRING];
  int iQty;
  double dCost;
  double dPrice;
  double dTotalProfit;
}sinventory;

Definitely not my style. If you are going to use typedef to provide a type name do not also provide a struct tag, and certainly not one all in uppercase. And look at those truly awful identifiers. Is ipnum a pointer to an int? No. But if you must name things this way, at least get the names consistent. ipnum should be iPnum. Actually I suspect that it shouldn t even be an int, I guess that ipnum and iptype are product codes that just happen to be written with digits and so should be stored as some kind of string. Finally I would much prefer the typedef typename was InventoryEntry or, if you must, sInventoryEntry.

sinventory  inventory;

And we finish with a definition of a global in a header file, even in C++ this means that the header file can only be used once per program.

And now to the main translation unit.

/****************************************
 This program gets inventory information for a 
sporting goods store from the user and saves it to 
disk. When all inventory items have been entered, 
the grand totals are displayed. A report function 
will print a report of items to screen for viewing.
****************************************/
#include <stdio.h>
#include <conio.h>
#include <ctype.h>
#include <string.h>
#include "c:\c++\ansic\l11\prodinv.h"

We haven t got passed the headers and we already have two points to note. conio.h is a DOS specific header file and would be much better placed after all the standard headers with a relevant comment to help others who might not recognise it. And then, why has the full path to prodinv.h been hard coded. And what about that curious 'C++' in the path? I would want to ask the writer quite a few questions just on the implications of that path.

void main() {

OK, so several compilers will fix up your code but main returns an int. It must do because it is returns to exit (that is the way that C code works) and that function requires a value for its status parameter.

  sinventory *sp;
  sp = &inventory;

First we had a global variable and now we have a pointer to it. Why? And even if you can justify that (which I doubt) why was it not written as:

  sinventory * sp = &inventory;

Why is the identifier sp? As we are using Hungarian notation that should mean a nameless pointer to short. Also note that the use of the identifier inventory would imply that this variable is some form of collection that represents the stock and not just a single stock item.

  /* local varaibles  */

Not only is this comment valueless, it is also wrong, because sp is also a local variable.

  char    cChoice;              /* user menu option */
  double  dGrandTotPrice = 0;   /* accumulator for price */
  double  dGrandTotCost = 0;    /*accumulator for cost */
  FILE    *fp;

More poor names. Why does the student adamantly refuse to give his file pointers descriptive names?

  if((fp = fopen("PRODUCTS.DAT","w+b")) == NULL)  {

Why is this file being opened in binary mode? Actually this is the cause of many of the problems that lead to the code being posted. The student uses the file inconsistently.

    printf("Unable to open file for writing; Exiting\n");
    getch();
  }

So you said you were exiting, so why not do so? Something like

exit(EXIT_FAILURE);

as a third statement in that compound statement block.

  else {

Of course this is not actually necessary if you call exit when you find that you have failed to open the required file.

  while((cChoice = cfnMenu()) != QUIT)  {
      /* get user choice */
      switch(cChoice) {
      case  ADD:  vfnAdd(&dGrandTotPrice,&dGrandTotCost,fp);
        break;
      case  REPORT:  vfnReport(sp,fp);
        break;
      case  DELETE:  break;
      case  CHANGE: break;
      }
    }                                   /* display grand totals */
    vfnGrandTotal(dGrandTotCost,dGrandTotPrice);
    fclose(fp);
    printf("\n\nPress any key to exit.\n");
    getch();
  }

And here we could do with either a return 0; or exit(EXIT_SUCCESS);

}

There are many hidden implications in this code (I have snipped the function definitions because Francis is not going to allow me space to inspect those as well). However the most important thing is that this is not code that has been written from scratch. The lab exercise is to modify already written code to use a file. So why has so much poor coding got passed the course director? And the following conclusion from the poster is even more disturbing. S/he has almost finished this course. The posting concludes with:

If only those teaching students to write C would do the job properly the students would find programming easier and we would have far less bad code being written. Note that the proposed use of delete as a function name conflicts with a C++ keyword. The resulting C code will not compile as C++. Sometimes this is acceptable but I could hardly justify it here.

There is another question that should be crossing your mind. Why is this young person learning C? C is a powerful language in its problem domains but I would not include database processing among them. I have a strong sense that this academic institution is wasting its students' time. You would be deeply disturbed by a medical school that taught blood letting as a standard medical practice (even though there are times when such radical old-fashioned techniques are useful.) The software industry needs to get its act together and communicate its needs to the universities and colleges.