Simple customer program in C (using binary files)

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
#include
#include

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.ac

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.

StackExchange Code Review Q#122359, answer score: 12