Simple customer program in C (using binary files)

Question

I have just finished a simple C program which is basicaly composed of 3 functions:

• accadd(void){} which adds some customer details to a binary file
• void view(char file[30]){} which is supposed to let me view some of the data in a .txt file
• void modify(char file[30]){} which verifies if a value of a struct is present in the binary file. If that value exists, it will replace it with the newly inserted one.

Now, I'm pretty new to C (coming from python world - should've learnt them in the opposite order ^__^) and I am sure that there are better ways of improving what I have just written. So I am looking for some advices / tips / improvements in my way of thinking + optimization of the code.

Here is the program:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
void menu(void);
void accadd(void);
void modify(char file[30]); 
void view(char file[30]);
// this should remain as it is
struct date
{
 int day;
 int month;
 int year;
};
// this should also remain as it is
struct customer
{
 char name[40], acctype[10];
 int accno, age;
 double phone;
 float amount;
 struct date deposit;
} add;
void accadd(void)
{
 FILE *fp = fopen("cus.dat", "ab+"); // open the binary file
 if (fp == NULL) // making sure that the file is ok
 exit(1);
 // start adding data
 printf("ADD RECORD\n");
 printf("Enter today's date(date/month/year) \n");
 if (scanf("%d/%d/%d", &add.deposit.day, &add.deposit.month, &add.deposit.year) != 3)
 exit(1);
 printf("Enter account number\n");
 if (scanf("%d", &add.accno) != 1)
 exit(1);
 printf("Enter customer's name\n");
 if (scanf("%s", add.name) != 1)
 exit(1);
 printf("Enter customer's age\n");
 if (scanf("%d", &add.age) != 1)
 exit(1);
 printf("Enter customer's phone num\n");
 if (scanf("%lf", &add.phone) != 1)
 exit(1);
 printf("Enter the account type(in words): \n\t 1:Current\n\t 2:Saving\n\t 3:Fixed\n");
 if (scanf("%s", add.acctype) != 1)
 exit(1);
 printf("Almost done! Just enter the amount you want to deposit: ");
 if (scanf("%f", &add.amount) != 1)
 exit(1);
 //add data to the binary file
 fwrite(&add, sizeof(add), 1, fp);
 fclose(fp);
}
// this is pretty straight-forward so I didn't add any comments on it
void view(char file[30])
{
 FILE *view, *output;
 int test = 0;
 output=fopen("output.txt", "w");
 fprintf(output,"Customer's List\n");
 fprintf(output,"\tCustomer's Name:");
 fprintf(output,"\tAccount Number:");
 fprintf(output,"\tCustomer's Phone No:\n");
 view = fopen(file, "rb");
 if (view == NULL)
 exit(1);
 while (fread(&add, sizeof(add), 1, view) != 0)
 {
 fprintf(output,"\t%16s", add.name);
 fprintf(output,"\t%15d", add.accno);
 fprintf(output,"\t%20.0f", add.phone);
 fprintf(output,"\n");
 test++;
 }
 fclose(view);
 fclose(output);
 if (test == 0)
 {
 printf("NO RECORDS FOUND!\n");
 }
 else{
 printf("Output updated in file output.txt\n");
 }
}
void modify(char file[30]){
 FILE *view;
 int counter = 0;
 view = fopen(file, "rb+");
 if (view == NULL)
 exit(1);
 while (fread(&add, sizeof(add), 1, view) != 0)
 {
 if(add.phone==777777777){
 printf("Old phone is: %f",add.phone);
 printf("Enter the new phone: ");
 scanf("%f", &add.phone);
 printf("\nThe new phone is: %f",add.phone);
 fseek(view,sizeof(add)*counter,SEEK_SET);
 fwrite(&add,sizeof(add),1,view);
 }
 counter++;
 }
 fclose(view);
 if (counter == 0)
 {
 printf("NO RECORDS FOUND!\n");
 }
 else{
 printf("add.phone printed: %i\n",counter);
 printf("add length %d\n",sizeof(add));
 }
}
void menu(void)
{
 int n,account_number;
 char file[30],account_name[30];
 printf("Enter your choice 1, 2, 3\n");
 printf("\n1. Insert new data about the customer");
 printf("\n2. Output data in output.txt file");
 printf("\n3. Modify phone nr.");
 printf("\n\nYour choice(1, 2, or 3): ");
 while(1){
 if (scanf("%d", &n) != 1)
 exit(1);
 switch (n){
 case 1:
 accadd();
 break;
 case 2:
 printf("Enter the file name: ");
 scanf("%s",&account_name);
 view(account_name);
 break;
 case 3:
 modify("cus.dat");
 break;
 }
 printf("Enter your choice 1, 2, 3\n");
 printf("\n1. Insert new data about the customer");
 printf("\n2. Output data in output.txt file");
 printf("\n3. Modify phone nr.");
 printf("\n\nYour choice(1, 2, or 3): ");
 }
}
int main(void)
{
 menu();
 return 0;
}

Answer 1

I see a number of things that may help you improve your code.

Eliminate function prototypes by ordering

Since you put the acadd and menu implementations above main in the source code, you don't need the function prototypes. Generally, I try to order functions from smallest and simplest to most complex so that one can read from the top to bottom. Your code is already structured that way.

Fix the format string errors

The code has this line:

scanf("%f", &add.phone);

But that line has two problems. First, scanf can fail so you should check the return code. Second, the %f signifies that a float is expected, but the code is actually reading into a double so the format string should be %lf. Make sure that the arguments match the format string types for both scanf and printf.

Don't use raw scanf for strings

The code that reads in an account name is currently this:

scanf("%s",&account_name);

This line has three problems. First, scanf can fail so you should check the return code. Second, the argument should simply be account_name and not &account_hame. Third, what happens if the name is longer than the allocated space? The result is a classic buffer overrun. You can easily prevent it using the length modifier:

scanf("%30s",account_name);

The code should also make sure that the string is properly terminated.

Eliminate unused variables

Unused variables are a sign of poor quality code, and you don't want to write poor quality code. In this code, file and account_number in menu() are unused. Your compiler is smart enough to tell you about this if you ask it nicely.

Eliminate "magic numbers"

There are a few numbers in the code, such as 30 and 40 that have a specific meaning in their particular context. By using named constants such as MAX_FILENAME_SIZE or MAX_NAME_SIZE, the program becomes easier to read and maintain. For cases in which the constant only has sense with respect to a particular object, consider making that constant part of the object.

Don't repeat yourself

The menu() text is printed in two different sections of code. Either consolidate that into a single function call or rearrange the code so that the text is only printed once at the top of each loop.

Add a default case

Each switch should generally have a default case. In your menu() function, the default case might be to tell the user that the choice isn't understood or to ask if the user intends to quit the program.

Think carefully about data types

Generally speaking, using a double for a phone and using a float for money are not good ideas. First, any phone number that begins with 0 is not going to be printed correctly and formatting (which is locale specific) is going to be lost. It's better to use a string for phone numbers. For money values, never use floats! because your program will suffer from rounding problems.

Think of the user

At the moment, there's no obvious graceful way to end the program. The user is trapped in an endless while(1) loop within menu. It would be nicer to add "Quit this program" as one of the menu choices rather than relying on the user to enter nothing.

Also, any data entry error on the part of the use in acadd() causes the program to suddenly and silently halt without even an error message and without a way to recover from the error.

Eliminate return 0 at the end of main

Since C99, the compiler automatically generates the code corresponding to return 0 at the end of main so there is no need to explicitly write it.

• Cajuu': thanks for your answer. I fixed almost everything you said but I have one question: Also, any data entry error on the part of the use in acadd().., what would you recommend here ?
• Edward: For data entry, think about what you'd like if you were the user. I'd want to be able to enter item at a time (with warning for any error) and then review the entire record before committing it. One way to do that would be to use fgets to get a string from the user and then try to convert it to whatever is needed. Keep looping until either the user gives a valid entry or indicates by some means that they want to abandon the effort.
• Cajuu': Nice tip. Didn't think of it. Can you please give me an example of usage in one or two blocks from my code ? ^__^ Just wanted to be sure that I'll hace it done the proper way
• Edward: See this question for one way to gracefully ask for integer input.

Answer 2

In addition to Edward's answer:

Missing return value checks

• output=fopen("output.txt", "w");

  This might fail as well. Try creating directory called "output.txt"

• scanf("%f", &add.phone);

• fseek(view,sizeof(add)*counter,SEEK_SET);

  Have You tried truncating it (the file) on the separate process? There are some more cases it could fail.

• fwrite(&add, sizeof(add), 1, view);

• while (fread(&add, sizeof(add), 1, view) != 0)

  According to fread() man page, it does not distinguish between error and end-of-file. So, You should use something like while (!feof(fread())

Library functions call exit()

I would debate that's not the way to handle errors. First of all, You're exiting application without taking care of all the allocated resources, opened files, etc. Also, how would You like if the library You're using would decide to exit in the middle of some... long-live process or heavy computing?

I'd say that returning error code or 0 on success is the way to go.

Also, You're introducing multiple exit points, which... does not sound for manageable code.

Poor "library" function naming

Your library function names (accadd(), modify(), view(), ...) do not indicate which specifically "object" are they designed for. Common "good" example would be something like struct buffer {}, buffer_init(), buffer_free()", buffer_add(), etc.

Global state

Why would You want to initiate and keep customer and date structures globally and statically? Having something like following would make Your code more versatile and error-prone (compiler's type checking):

struct customer {};
struct customer *customer_create();
void customer_destroy(struct customer *);
...

const const const const

Why would You keep char's mutable if You're not modifying those anywhere in Your functions?

I would change both, void view(char file[30]) and void modify(char file[30]) to void view(const char file[30]) and void modify(const char file[30])

Note:

I would leave the proper return value of the main() function. Despite Edward's comment about new C standards. Really, I need a quote for that.