469,358 Members | 1,642 Online
Bytes | Developer Community
New Post

Home Posts Topics Members FAQ

Post your question to a community of 469,358 developers. It's quick & easy.

How can I improve these functions?

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
Nov 14 '05 #1
4 1448

"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.
Nov 14 '05 #2

"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.
Nov 14 '05 #3
Thankyou for your suggestions Malcolm, as well as being more failsafe, my
functions are a lot more portable now.

Cheers

Sean
Nov 14 '05 #4
Thankyou for your suggestions Malcolm, as well as being more failsafe, my
functions are a lot more portable now.

Cheers

Sean
Nov 14 '05 #5

This discussion thread is closed

Replies have been disabled for this discussion.

Similar topics

10 posts views Thread by pembed2003 | last post: by
17 posts views Thread by Sean Kenwrick | last post: by
reply views Thread by S Kemplay | last post: by
9 posts views Thread by Peng Jian | last post: by
1 post views Thread by lostbuttrying | last post: by
75 posts views Thread by At_sea_with_C | last post: by
reply views Thread by zhoujie | last post: by
1 post views Thread by Marylou17 | last post: by
By using this site, you agree to our Privacy Policy and Terms of Use.