Phonebook implementation in C

Question

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 ("\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 Contacts ?  : ") ;
        if ( scanf ("%d" , &m ) !=1 )
            break ;
        else
        input_flush() ;
        while ( m>0 )
        {
            if ( ! get_details() ) add_contact() ;
            m-- ;
        }
        break ;

    case '3':
        printf (" \n Enter Part/Full Contact Name/Number  : ") ;
        gets(temp_name ) ;
        search_contact() ;
        break ;

    case '4':
        printf (" \n Enter Full Contact Name : ") ;
        gets(temp_name ) ;
        delete_contact() ;
        break ;

    case '5':
        printf (" \n Entire Data Will Be Lost !    Continue ?? { Y/other } : ") ;
        ch = getchar () ;  getchar() ;
        if ( ch == 'Y' || ch == 'y' )
        {
            format_book( &head ) ;
            printf ("\n Successful ! ")  ;
        }
        else
            printf ("\n Aborted ! ") ;
        break ;

    case '6':
        print_book() ;  break ;

    default :
        printf ("\n\n\nThank 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("\n\n Phone Book is Empty ..! ") ;
        return ;
     }
        /* String to be searched is read to ' temp_name'  */

     while ( temp!= NULL )
     {
        if ( strstr (temp->name , temp_name ) !=NULL  ||  strstr(temp->no , temp_name )!=NULL )
        {
            print_record( temp ) ;    /* Detail Matched */
            cnt++ ;
        }
        temp=temp->next  ;
     }

     footer () ;
     printf ("\n\n %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("\n\n Phone Book is Empty ..! ") ;
        return ;
      }
      if ( strcmp( head->name , temp_name ) == 0 )
      {
        address = head ->next ;
        free( head )  ;
        head =  address  ;
        printf("\n\n Contact deleted successfully...!! ") ;
        return ;
      }
      while ( temp->next !=NULL )
      {
        if ( strcmp (temp->next->name , temp_name ) == 0 )
        {
            address =  temp ->next->next ;
            free ( temp->next ) ;
            printf("\n\n Contact deleted successfully...!! ") ;
            temp->next  = address  ;
            return ;
        }
        else
            temp=temp->next  ;
      }
      printf ("\n\n 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("\n\n 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] ='\0', 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] = '\0' ;

    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 , "%s\n" , 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("\n\n 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 ( )
{
    char * name = temp_name ;
    char * no  = temp_no ;

    if ( strlen ( temp_name ) > MAX_NAME ||  strlen( temp_name ) <1 )
    {
        printf("\n\n Failed !\n Length exceeded/ No input for name { max 25 } \n") ;
        return  1;
    }
    for (   ; *name  ;  name ++ )

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

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

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

    lowercase ( temp_name ) ;

    if ( !duplicate_check() )
    {
        printf ("\n\n Successful ! \n ") ;
        return  0;
    }

}

int duplicate_check( )
{
    phonebook * temp = head ;

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

Edward · Accepted Answer · 2016-01-26 00:15:00Z

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.

Thank you so much @Edward , I will work on this Improvements for being a better programmer . Thank you :) — venugopal reddy, Jan 5 at 17:05
Hello Edward, if we make temp_name and temp_no as local variable and Head node too , we have to pass all the three parameters to each and every function. wont that be clumsy ? Like the Following void add_contact( phonebook ** node , char * temp_name , char *temp_no);' — venugopal reddy, Feb 5 at 15:28
@venugopalreddy: Yes, passing all three parameters is the way to do it. Right now you effectively "pass" all three but two parameters are global and can't be determined by just looking at the interfaces. That's not good. Better is to make it simple to see what the actual linkage is, and that's best done by eliminating the vague linkage of global variables and making it explicit and clear by passing parameters. — Edward, Feb 5 at 19:57
ok. I think I got it right . parameters make a function explicit and helps to understand the linkage b/w code and the external data we manipulate in the function. And secured too.. — venugopal reddy, Feb 5 at 20:26
Hello Edward, I have revised the whole program by keeping every suggestion In mind. could you please say few words on this version.. The link is here - [drive.google.com/… or I could edit here if its ok. — venugopal reddy, Feb 11 at 0:57

barak manos · Answer 2 · 2016-01-05 12:36:59Z

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
}

thank you @barak manos. I wil take the suggessions that you have said. i forgot to delete the unused variable 'c' . I will try to minimize the global variables. and create them wisely . — venugopal reddy, Jan 5 at 12:53

asked	4 months ago
viewed	310 times
active	2 months ago

current community

your communities

more stack exchange communities

Phonebook implementation in C

2 Answers 2

Comment unusual code

Don't use `gets`

Avoid the use of global variables

Eliminate unused variables

Use const where practical

Don't use `system("cls")`

Don't put code in headers

Make sure all paths return a value

Use consistent formatting

Be careful with signed versus unsigned numbers

Choose better names

Check your return values

Avoid being overly restrictive of inputs

Think of the user

Your Answer

Not the answer you're looking for? Browse other questions tagged c data-structures or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Phonebook implementation in C

2 Answers 2

Comment unusual code

Don't use gets

Avoid the use of global variables

Eliminate unused variables

Use const where practical

Don't use system("cls")

Don't put code in headers

Make sure all paths return a value

Use consistent formatting

Be careful with signed versus unsigned numbers

Choose better names

Check your return values

Avoid being overly restrictive of inputs

Think of the user

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c data-structures or ask your own question.

Related

Hot Network Questions

Don't use `gets`

Don't use `system("cls")`