Phonebook implementation in C

I have implemented a phonebook in C using a singly linked list sorted data structure. This is my first average-major project.

I need some reviews so that I can improve my coding standards. The program consists of 3 files:

header.h: custom header

/*
 * File : header.h
 * Custom Header Definition
 */

 #include
 #include
 #include
 #include

 #ifdef __linux__
 #define CLEAR_SCREEN system("clear")
 #elif _WIN32
 #define CLEAR_SCREEN system("cls")
 #endif

 #define  MAX_NAME  26
 #define  MAX_NO  11

 typedef struct phonebook {
 char * name  ;
 char * no ;
 struct phonebook * next ;

 } phonebook ;

 /* Root Node */
 extern  phonebook * head ;

 /* Temporary Storage   */    
 extern char temp_name[] ;
 extern char temp_no[] ;

 /* Serial no while printing */
 extern int START ;

 /* Gets Input values From User  , Returns 1 if it Fails */
 extern int get_details() ;

 /* Flushing stdin */
 extern void input_flush() ;

 /* basic verbs  */

 extern void add_contact ( ) ;
 extern void print_book () ;
 extern void search_contact (  ) ;
 extern void delete_contact ( ) ;
 extern void format_book ( struct phonebook ** ) ;

 /* File Operations */

 extern void file_read() ;
 extern void file_write() ;

el.c: execution logic

```
#include "header.h"

char temp_name [ MAX_NAME ] ;
char temp_no [ MAX_NO ] ;
int START = 1 ;

phonebook * head = NULL ;

int run ()
{
char ch ;
char c ; / flush /
int m =0 ;
CLEAR_SCREEN ;

printf ("\n\n\t PHONEBOOK MANAGER \n") ;
printf ("\n\n\n 1 . %-12s " , "Add Contact") ;
printf ("\n\n 2 . %-12s " , "Multiple") ;
printf ("\n\n 3 . %-12s " , "Search") ;
printf ("\n\n 4 . %-12s " , "Delete") ;
printf ("\n\n 5 . %-12s " , "Format") ;
printf ("\n\n 6 . %-12s " , "Print All") ;
printf ("{ Choice : }\b\b\b") ;

ch = getchar() ;

input_flush() ;
CLEAR_SCREEN ;

switch ( ch )
{
case '1':
if ( ! get_details() ) add_contact();
break ;

case '2':
printf (" \n How many Contac

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

Comment unusual code

The code currently contains this loop:

while ( temp_no[++i] = *str )
        str++ ;

This code might do what you intend, but at first glance, it looks like it might be an error because it's = instead of ==. It's probably a good idea to add a comment there explaining what's being done and why.

Don't use gets

Using gets is not good practice because it can lead to buffer overruns. It has been removed from the C11 standard and marked "obsolete" in POSIX 2008. Use fgets instead, which requires the specification of a buffer size.

Avoid the use of global variables

It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable. The temp_name and temp_no variables in particular, are especially worthy of being eliminated. By their names, they should likely have been local to particular functions that need them rather than global.

Eliminate unused variables

This code declares a variable response within get_details() but then does nothing with it. The same is true of c within run(). Your compiler is smart enough to help you find this kind of problem if you know how to ask it to do so.

Use const where practical

The current print_record() routine does not (and should not) modify the passed phonebook struct, and so it should be declared const:

void print_record(const phonebook *);

Don't use system("cls")

I usually give two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems, but I see that you have attempted to address that one. However, the second is more important: it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. Rewrite the function to do what you want using C and avoiding system. For example, if your terminal supports ANSI Escape sequences, you could use this:

void cls()
{
    printf("\x1b[2J");
}

Don't put code in headers

The current CLEAR_SCREEN code exists solely in a header, but that's not the right place. Header files shouldn't have code in them. Headers should only include declarations of functions and variables and structures, but not code. In this case, it's a simple matter to just move that function to the el.c file which is the only place it's used.

Make sure all paths return a value

The error_check() routine checks for various errors and returns a value, but there is a control path through the function that gets to the bottom of the function. The function should return a value no matter what control path is executed.

Use consistent formatting

The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.

Be careful with signed versus unsigned numbers

If you only want to allow positive numbers for the number of contacts in run(), which makes sense in this context, that should be declared as unsigned rather than int. Similarly, you can use %u in printf and scanf statements rather than %d.

Choose better names

The variable m within run() is used as the number of contacts. Nothing about the name m suggests that meaning; it would be better to call it contact_count or similar. Also the header really should be named something better than header.h.

Check your return values

The code in file_read() calls fopen and then immediately does a getc. What if either of those calls fails? The code doesn't currently check the return value, which could indicate a failure. Better would be to check the return value and do the appropriate thing -- possibly by returning early. Similarly, calls to malloc can fail and that situation should be handled gracefully.

Avoid being overly restrictive of inputs

The current code requires exactly ten digits for a mobile phone number and only allows numbers starting with the digits '8' or '9'. This makes the program very specific and unusable on most of the planet. Rather than rejecting other phone numbers, it might instead be better to accept them. If you need an error message, make it a warning rather than an error.

Think of the user

If I decide to put in the number of my friend Jürgen, I find that even though I get no error message, his number is not put into the phonebook. You should investigate why that is and fix it.

StackExchange Code Review Q#115880, answer score: 3