How can I improve these functions?

[S Kemplay]

Hi all,

I am after any pointers you may have to improve these two functions.

They both work but are part of an assignment - I am looking for feedback.

The first one reads records from a file - the teacher won't allow a
delimeted file he wants spaces between words joined with an underscore.

The records are read into an array of structs:

typedef struct drink
{
int*stockCode;**************************
****char*drinkName[45];*********************
****char*manufacturer[25];**********
****float*purchasePrice;
} DRINK;

void option1(DRINK drinkArray[], DRINK drinkRec)
{
****FILE**fp*=*fopen("products.txt",*"r");
****char*line[255]*=*"";
****int*count*=*0;
****printf("\nRead*records*from*file\n");
****printf("======================\n\n");
****if(fp)
****{
********while((fgets(line,*sizeof(line),*fp)*!=*NU LL))
********{
************sscanf(line,*"%d*%44s*%24s*%f\n",*&dri nkRec.stockCode,
************drinkRec.drinkName,**drinkRec.manufact urer,
************&drinkRec.purchasePrice);
************if(addDrinkArray(*drinkArray,*drinkRec ))
****************++count;
************else
************{
****************printf("Array*is*full:*");
****************break;
************}
********}
****fclose(fp);
****}
****else
********printf("Error*opening*file");

****printf("%d*records*succsefffully*added\n",*cou nt);
}

The second function is to validate user input - I need a float for
purchasePrice. I have a similar one for an int.

I want to be able to accept a newline as valid when this function is used to
get input to update a record ie "Enter a purchase price or press enter to
leave unchanged" so I have set a flag. If the flag is true, the return
signals whether a newline was entered or not.

int getValidPurchasePrice(float* purchasePrice, int bypass)
{
****char*line[255];
****char**test;
****int*retval*=*1;
****while(1)
****{
********printf("Please*enter*the*purchase*price*") ;
********fgets(line,*sizeof(line),*stdin);
*********purchasePrice*=*strtod(line,*&test);
********if(isspace(*test)*&&*line[0]*!=*'\n')
********{
*************break;
********}
********else*if(line[0]*==*'\n'*&&*bypass*!=*0)
********{
************retval*=*0;
************break;
********}
********line[strlen(line)*-*1]*=*'\0';
********printf("%s*is*not*a*valid*purchase*price\n ",*line);
****}
****return*retval;
}
Thanks in advance

Sean Kemplay

[Malcolm]

"S Kemplay" <sk******@dingley.net> wrote in message

I am after any pointers you may have to improve these two
functions.

typedef struct drink
{
int stockCode;
char drinkName[45];
char manufacturer[25];
float purchasePrice;
} DRINK;
/*
First I'll give you local improvements, then I'll explain how to redesign.
*/ void option1(DRINK drinkArray[], DRINK drinkRec)
{
FILE* fp = fopen("products.txt", "r");
char line[255] = "";
int count = 0;
printf("\nRead records from file\n");
printf("======================\n\n");
if(fp)
{
while((fgets(line, sizeof(line), fp) != NULL)) /*
You want to check that a full line was read by looking for the newline.
*/ {
sscanf(line, "%d %44s %24s %f\n", &drinkRec.stockCode,
drinkRec.drinkName, drinkRec.manufacturer,
&drinkRec.purchasePrice);
sscanf() returns the number of items successfully converted. Check the
return value and bale if the record is malformed.
if(addDrinkArray( drinkArray, drinkRec))
++count;
else
{
printf("Array is full: "); /*
This is a bit lame. You need to be able to read records until the computer
runs out of memory.
*/ break;
}
}
fclose(fp);
}
else
printf("Error opening file"); /*
You might consider printing errors to stderr.
*/
printf("%d records succsefffully added\n", count);
}
Why not rewrite the function

/*
Returns an allocated array of N drinks read from the file.
*/
DRINK *readdrinks(char *fname, int *N)
{
}
Also, you could get rid of all the output to printf() and put it somewhere
else, so the function is more portable.
The second function is to validate user input - I need a float for
purchasePrice. I have a similar one for an int.

I want to be able to accept a newline as valid when this function is used to get input to update a record ie "Enter a purchase price or press enter to
leave unchanged" so I have set a flag. If the flag is true, the return
signals whether a newline was entered or not.

int getValidPurchasePrice(float* purchasePrice, int bypass)
{
char line[255];
char* test;
int retval = 1;
while(1)
{
printf("Please enter the purchase price ");
fgets(line, sizeof(line), stdin);
*purchasePrice = strtod(line, &test);
if(isspace(*test) && line[0] != '\n')
{
break;
}
else if(line[0] == '\n' && bypass != 0)
{
retval = 0;
break;
}
line[strlen(line) - 1] = '\0';
printf("%s is not a valid purchase price\n", line);
}
return retval;
}

Write the function

int validmoney(const char *str, double minprice, double maxprice)
{
Call strtod. Check for garbage following what you read. This tells
you you have a valid floating-point number.
Checking for min price and max price is trivial (min might be 1 cent, max
can be DBL_MAX or you might want to sanity check).
However 12 and 12.34 are valid, 12.3 and 12.345 are not. You need to use
strchr() to seach for the decimal point and, if it exists, count the digits
(using isdigit). If there are two then you are OK, if not then reject it.
}

Input can also be invlaid if fgets() fails to read a line. keep this
separate from your validmoney() function.

[Malcolm]

"S Kemplay" <sk******@dingley.net> wrote in message

I am after any pointers you may have to improve these two
functions.

typedef struct drink
{
int stockCode;
char drinkName[45];
char manufacturer[25];
float purchasePrice;
} DRINK;
/*
First I'll give you local improvements, then I'll explain how to redesign.
*/ void option1(DRINK drinkArray[], DRINK drinkRec)
{
FILE* fp = fopen("products.txt", "r");
char line[255] = "";
int count = 0;
printf("\nRead records from file\n");
printf("======================\n\n");
if(fp)
{
while((fgets(line, sizeof(line), fp) != NULL)) /*
You want to check that a full line was read by looking for the newline.
*/ {
sscanf(line, "%d %44s %24s %f\n", &drinkRec.stockCode,
drinkRec.drinkName, drinkRec.manufacturer,
&drinkRec.purchasePrice);
sscanf() returns the number of items successfully converted. Check the
return value and bale if the record is malformed.
if(addDrinkArray( drinkArray, drinkRec))
++count;
else
{
printf("Array is full: "); /*
This is a bit lame. You need to be able to read records until the computer
runs out of memory.
*/ break;
}
}
fclose(fp);
}
else
printf("Error opening file"); /*
You might consider printing errors to stderr.
*/
printf("%d records succsefffully added\n", count);
}
Why not rewrite the function

/*
Returns an allocated array of N drinks read from the file.
*/
DRINK *readdrinks(char *fname, int *N)
{
}
Also, you could get rid of all the output to printf() and put it somewhere
else, so the function is more portable.
The second function is to validate user input - I need a float for
purchasePrice. I have a similar one for an int.

I want to be able to accept a newline as valid when this function is used to get input to update a record ie "Enter a purchase price or press enter to
leave unchanged" so I have set a flag. If the flag is true, the return
signals whether a newline was entered or not.

int getValidPurchasePrice(float* purchasePrice, int bypass)
{
char line[255];
char* test;
int retval = 1;
while(1)
{
printf("Please enter the purchase price ");
fgets(line, sizeof(line), stdin);
*purchasePrice = strtod(line, &test);
if(isspace(*test) && line[0] != '\n')
{
break;
}
else if(line[0] == '\n' && bypass != 0)
{
retval = 0;
break;
}
line[strlen(line) - 1] = '\0';
printf("%s is not a valid purchase price\n", line);
}
return retval;
}

Write the function

int validmoney(const char *str, double minprice, double maxprice)
{
Call strtod. Check for garbage following what you read. This tells
you you have a valid floating-point number.
Checking for min price and max price is trivial (min might be 1 cent, max
can be DBL_MAX or you might want to sanity check).
However 12 and 12.34 are valid, 12.3 and 12.345 are not. You need to use
strchr() to seach for the decimal point and, if it exists, count the digits
(using isdigit). If there are two then you are OK, if not then reject it.
}

Input can also be invlaid if fgets() fails to read a line. keep this
separate from your validmoney() function.

[S Kemplay]

Thankyou for your suggestions Malcolm, as well as being more failsafe, my
functions are a lot more portable now.

Cheers

Sean

[S Kemplay]

Thankyou for your suggestions Malcolm, as well as being more failsafe, my
functions are a lot more portable now.

Cheers

Sean