How can I improve these functions?

S

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) != NULL))
        {
            sscanf(line, "%d %44s %24s %f\n", &drinkRec.stockCode,
            drinkRec.drinkName,  drinkRec.manufacturer,
            &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", count);
}

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
 
M

Malcolm

S Kemplay said:
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

S Kemplay

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

Cheers

Sean
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,484
Members
44,903
Latest member
orderPeak8CBDGummies

Latest Threads

Top