Ttykuu

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<stdio.h>
 #include<string.h>
 #include<stdlib.h>
 #include<ctype.h>

 #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 ("nnt PHONEBOOK MANAGER n") ;
printf ("nnn 1 . %-12s " , "Add Contact") ;
printf ("nn 2 . %-12s " , "Multiple") ;
printf ("nn 3 . %-12s " , "Search") ;
printf ("nn 4 . %-12s " , "Delete") ;
printf ("nn 5 . %-12s " , "Format") ;
printf ("nn 6 . %-12s " , "Print All") ;
printf (" Choice : bbb") ;

ch = getchar() ;

input_flush() ;
CLEAR_SCREEN ;

switch ( ch )
 ch == 'y' )
 
 format_book( &head ) ;
 printf ("n Successful ! ") ;
 
 else
 printf ("n Aborted ! ") ;
 break ;

 case '6':
 print_book() ; break ;

 default :
 printf ("nnnThank You ! n") ;
 return 0;


input_flush() ;

return 1 ;


int main ()

 file_read() ;
 while ( run () ) ;
 file_write() ;
 return 0 ;

bl.c: contain Implementations of Subroutines

#include"header.h"

/* helping subroutines */

void new_node ( phonebook ** ) ;
void fill_details () ;

void print_record ( phonebook * ) ;
void header () ;
void footer () ;

int duplicate_check() ;
int error_check () ;
void split (char * ) ;

void lowercase (char * ) ;
void input_flush() ;


void add_contact ( )

 /* Traversing */
 phonebook *temp = head ;

 /* Storing Temporary Address */
 phonebook * address = NULL ;

 /* Adding First Contact */

 if ( temp == NULL )
 
 new_node ( &head ) ;
 fill_details ( head ) ;
 return ;
 

 /* Modifying Root Node */

 if ( strcmp ( temp->name, temp_name ) >0 )
 
 new_node ( &head ) ;
 fill_details ( head ) ;
 head->next = temp ;
 return ;
 

 /* Adding Upcoming */

 while ( temp->next !=NULL )
 
 if ( strcmp( temp->next->name , temp_name ) < 0 )
 temp=temp->next ;
 else
 break ;
 

 /* Contact to add in the middle of two nodes */

 if ( temp->next != NULL )
 
 address = temp->next ;
 new_node ( &temp->next ) ;
 fill_details ( temp->next ) ;
 temp->next->next = address ;
 return ;
 
 /* Adding contact at the end ( appending node at the end ) */
 else
 
 new_node ( &temp->next ) ;
 fill_details ( temp->next ) ;
 temp->next->next = NULL ;
 return ;
 



void search_contact ()

 phonebook * temp = head ;

 /* How many contacts matched */
 int cnt =0 ;

 header () ;
 START = 1 ;

 if ( head ==NULL)
 
 footer () ;
 printf("nn Phone Book is Empty ..! ") ;
 return ;
 
 /* String to be searched is read to ' temp_name' */

 while ( temp!= NULL )
 
 if ( strstr (temp->name , temp_name ) !=NULL 

 footer () ;
 printf ("nn %d Contact%s have been Matched ! n " , cnt , cnt==1?"":"s" ) ;


void delete_contact( )

 /* Traversing */
 phonebook * temp = head ;

 /* Storing Temporary Address */
 phonebook * address = NULL ;

 if ( head ==NULL)
 
 printf("nn Phone Book is Empty ..! ") ;
 return ;
 
 if ( strcmp( head->name , temp_name ) == 0 )
 
 address = head ->next ;
 free( head ) ;
 head = address ;
 printf("nn Contact deleted successfully...!! ") ;
 return ;
 
 while ( temp->next !=NULL )
 
 if ( strcmp (temp->next->name , temp_name ) == 0 )
 
 address = temp ->next->next ;
 free ( temp->next ) ;
 printf("nn Contact deleted successfully...!! ") ;
 temp->next = address ;
 return ;
 
 else
 temp=temp->next ;
 
 printf ("nn Contact Does not Exist ! n ") ;


void print_book ( )


 phonebook *temp = head ;

 /* Serial no is reset to 1 */
 START = 1 ;
 printf ("n Complete List n") ;
 header () ;

 if ( head ==NULL)
 
 footer () ;
 printf("nn Phone Book is Empty ..! ") ;
 return ;
 
 while ( temp!= NULL)
 
 print_record( temp ) ;
 temp = temp->next ;
 
 footer () ;


void file_read ()


 char data[44] ;
 int i=-1 ;
 char ch ;
 FILE * fp1 ;
 fp1 = fopen ("contacts.txt" , "r") ;

 while ( ( ch = getc (fp1) ) != EOF )
 
 if ( ch == 'n')
 
 data[++i] ='', split (data) ;
 add_contact() ;
 i = -1 ;
 
 else
 
 data[++i] = ch ;
 
 
 fclose( fp1 ) ;
 remove ( "contacts.txt") ;



/* Separate Name and No. from "Data" store them in temp_name , temp_no */

void split (char * str )


 int i=-1 ;
 while ( *str!='=' )
 
 temp_name[++i] = *str ;
 str++ ;
 
 temp_name[++i] = '' ;

 str++ ;
 i=-1 ;

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


void file_write ()

 phonebook * temp = head ;
 char data[40] ;
 FILE * fp1 ;
 fp1 = fopen ("contacts.txt" , "w") ;

 while ( temp != NULL )
 
 strcpy ( data , temp->name ) ;
 strcat ( data , "=") ;
 strcat ( data , temp->no ) ;
 fprintf ( fp1 , "%sn" , data) ;
 temp=temp->next ;
 
 fclose ( fp1 ) ;



void format_book ( phonebook ** temp )

 if ( *temp == NULL )
 return ;
 format_book ( &( (*temp)->next ) ) ;
 free ( *temp ) ;
 *temp=NULL ;


int get_details ()

 int response = 0 ;
 printf("nn Enter Contact Name : " ) ;
 gets(temp_name) ;
 printf("n Enter Mobile Number : " ) ;
 gets(temp_no) ;

 return error_check() ;


void new_node ( phonebook ** temp )

 *temp = ( phonebook * ) malloc ( sizeof ( head ) ) ;
 (*temp)->name = NULL ;
 (*temp)->no = NULL ;
 (*temp)->next = NULL ;
 return ;


void fill_details ( phonebook * temp )

 temp->name = ( char* ) malloc ( sizeof (char) * MAX_NAME ) ;
 temp->no = ( char* ) malloc ( sizeof (char) * MAX_NO ) ;
 strcpy ( temp->name , temp_name ) ;
 strcpy ( temp->no , temp_no ) ;
 return ;



void input_flush() 

 int c ;
 while((c = getchar()) != 'n' && c != EOF) ;



void header ()

 printf("n %-50s" , "------------------------------------------------------- " ) ;
 printf("n %-8s %-29s %-11s" , "SL.NO" ,"CONTACT NAME " , "MOBILE NUMBER " ) ;
 printf("n %-50s" , "------------------------------------------------------- " ) ;




void footer ()

 printf("n %-50s" , "------------------------------------------------------- " ) ;



void print_record ( phonebook * temp )

 printf ("n %2d %-29s %-11s" , START , temp->name , temp->no ) ;
 START++ ;



/* Arbitrary for strlwr */
void lowercase ( char * temp )

 while ( *temp ) 
 
 if (*temp>='A' && *temp<='Z' ) 
 *temp= *temp+32 ;
 temp++ ;
 


int error_check ( )
 strlen( temp_name ) <1 )
 
 printf("nn Failed !n Length exceeded/ No input for name max 25 n") ;
 return 1;
 
 for ( ; *name ; name ++ )

 if ( !isalpha (*name ) && *name != ' ' )
 
 printf( "nn Failed !n Invalid characters used for name only alphabet , space allowed n" ) ;
 return 1;
 
 if ( strlen ( temp_no)!=10 )
 
 printf("nn Failed !n Invalid ten digit number Ten Digits Please .n") ;
 return 1;
 

 if ( *temp_no <'7' )
 
 printf("nn Failed !n Currently No Numbers Exist on '%c' Series .n" , *temp_no ) ;
 return 1;
 
 for ( ; *no ; no++ )

 if ( ! isdigit (*no ) )
 
 printf( "nn Failed !n Invalid characters used for number only digits allowed n" ) ;
 return 1;
 

 lowercase ( temp_name ) ;

 if ( !duplicate_check() )
 
 printf ("nn Successful ! n ") ;
 return 0;
 



int duplicate_check( )

 phonebook * temp = head ;

 while (temp!= NULL )
 
 if ( strcmp( temp->name , temp_name )==0 )
 
 printf ("nn Failedn Contact Exists with Same Name ! ") ;
 return 1 ;
 
 if ( strcmp( temp->no , temp_no )==0 )
 
 printf ("nn Failedn Number Associated with " %s ". " , temp->name ) ;
 return 1 ;
 
 temp=temp->next ;
 
 return 0 ;

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.

Do not ignore compiler warnings:

• Function run: variable c is unreferenced.


• Function error_check: not all control paths return a value.

Header files are typically used as a programming-interface. Users assume that whatever is defined or declared in a header file can be used. In fact, sometimes a header file doesn't even come along with a corresponding source file, but with an already-compiled code (aka library file). Having said that:

• In general, it is preferable to create one header file per each source file. This is how we conventionally expose our interface to other users. Here, you have created a single header file containing everything that is common to both source files. Ideally, you should create files bl.h and el.h, and have each one of these files reflecting the interface that you wish to provide within the corresponding source file.


• The declaration of head can be removed from the header file. You can instantiate this variable in file bl.c (as static, so no one will attempt to extern it elsewhere). You can then avoid passing it to function format_book.


• The declaration of START can be removed from the header file. You can instantiate this variable in file bl.c (as static, so no one will attempt to extern it elsewhere).


• Try to avoid declaring temp_name and temp_no as global variables. Instead, declare them locally in function run, and pass each one of them to any other function that makes use of it.


• Remove the CLEAR_SCREEN definition from the header file, and implement it as a function in file el.c:

void ClearScreen()

#ifdef __linux__
 system("clear")
#elif _WIN32
 system("cls")
#else
 #error "Platform not defined"
#endif

why this code does not how contact.txt file in which whole data is store