2
\$\begingroup\$

I'm trying to organize the code according to the run menu of a schedule using the binary tree data structure because if I search for a user, the menu always appears first, and the listed user is left behind. How do I organize the source code correctly of this schedule in C?

The C code of the agenda tree:

file: phonebook.h

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

struct Arvore
{
 char name[100];
 int phone;
 struct Arvore *esq;
 struct Arvore *dir;
};

typedef struct Arvore Agenda;

void toUpper(char str[]);
void option(char op[]);
void showContacts();
void getName(char *name);
void getPhone(int *phone);
void menuPhonebook(int *num);
void order(Agenda *raiz);
Agenda** maior_esq(Agenda *raiz);
Agenda** menor_dir(Agenda *raiz);
void searchContact(Agenda *raiz, char str[]);
void insertContact(Agenda **raiz, char str[]);
void changeContact(Agenda **raiz, char str[]);
void deleteContact(Agenda **raiz, char str[]);
void message1();
void message2();
void message3();
void message4();
void message5();
void message6();
void message7();
void message8();

file: phonebook.c

#include <string.h>
#include "phonebook.h"

int strcasecmp(const char *s1, const char *s2);

void insertContact(Agenda **raiz , char str[50])
{
 int phone;

 if (*raiz == NULL)
  {
   *raiz = (Agenda*)malloc(sizeof(Agenda));
   
   strcpy((*raiz)->name, str);
   
   getPhone(&phone);
   
   (*raiz)->phone = phone;
   (*raiz)->esq = NULL;
   (*raiz)->dir = NULL;              
  }
 else
  {
   if (strcasecmp((*raiz)->name,str) > 0)
    {
     insertContact(&(*raiz)->esq, str);                     
    }
   else
    {
     if (strcasecmp((*raiz)->name,str) < 0)
      {
       insertContact(&(*raiz)->dir, str);           
      }
     else
      {
       if (strcmp((*raiz)->name,str) == 0)
        {
         printf("NAME ALREADY EXISTS!\n");
        }
      }
    }
  }
}
                                                               
void order(Agenda *raiz)
{
 if (raiz!=NULL)
  {
   order((raiz)->esq);
   
   printf("\n\tNAME: %s", (raiz)->name);
   printf("\tPHONE: %d\n", (raiz)->phone);

   order((raiz)->dir);
  }
}

void searchContact(Agenda *raiz, char str[50])
{
 if (raiz != NULL)
  {
   if (strcasecmp((raiz)->name,str) > 0)
    {
     searchContact((raiz)->esq, str);
    }
   else
    {
     if (strcasecmp((raiz)->name,str) < 0)
      {
       searchContact((raiz)->dir, str);
      }
     else
      {
       if (strcmp((raiz)->name,str) == 0)
        {
         message6();
         printf("\n\tNAME: %s", (raiz)->name);
         printf("\tPHONE: %d\n", (raiz)->phone);
        }
      }
    }
  }
 else
  {
   message8();
  }
}

void changeContact(Agenda **raiz, char *str)
{
 if ((*raiz) != NULL)
  {
   if (strcasecmp((*raiz)->name, str) > 0)
    {
     changeContact(&(*raiz)->esq, str);
    }
   else
    {
     if (strcasecmp((*raiz)->name, str) < 0)
      {
       changeContact(&(*raiz)->dir, str);
      }
     else
      {
       if (strcmp((*raiz)->name, str) == 0)
        {
         int phone;
         message6();
         
         printf("\n\tNAME: %s", (*raiz)->name);
         scanf(" %[^\n]s",(*raiz)->name);
         
         printf("\n\tNEW PHONE: ");
         scanf(" %d",&phone);

         (*raiz)->phone = phone;
         message8();
        }
      }
    }
  }
 else
  {
   message8();
  }
}

void deleteContact(Agenda **raiz, char str[])
{
 Agenda **aux2, *aux3;

 if (*raiz != NULL)
  {
   if (strcasecmp((*raiz)->name , str) == 0)
    {
     if ((*raiz)->esq == (*raiz)->dir)
      {
         free(*raiz);
         *raiz = NULL;
      }
     else
      {
       if ((*raiz)->esq != NULL)
        {
         aux2 = maior_esq(*raiz);
         aux3 = *aux2;
         (*aux2) = (*aux2)->esq;
        }
       else
        {
         aux2 = menor_dir(*raiz);
         aux3 = *aux2;
         (*aux2) = (*aux2)->dir;
        }
       
       strcpy((*raiz)->name, aux3->name);
       free(aux3); 
       aux3 = NULL;
     }
    }
   else
    {
     if (strcasecmp(str,(*raiz)->name) < 0)
      {
       deleteContact(&(*raiz)->esq, str);
      }
     else
      {
       deleteContact(&(*raiz)->dir, str);
      }
    }
  }
 else
  {
   message8();
  }
}

Agenda** maior_esq(Agenda *raiz)
{
 Agenda **aux = &(raiz);
 
 if ((*aux)->esq != NULL)
  {
   aux = &(*aux)->esq;

   while((*aux)->dir != NULL)
   {
    aux = &(*aux)->dir;
   }
  }
 
 return aux;
}

Agenda** menor_dir(Agenda *raiz)
{
 Agenda **aux = &(raiz);

 if ((*aux)->dir != NULL)
  {
   aux = &(*aux)->dir;

   while((*aux)->esq != NULL)
   {
    aux = &(*aux)->esq;
   }
  }
 
 return aux;
}

void menuPhonebook(int *num)
{
 printf("\033[2J\033[H");
 printf("\t     *-------------*\n");
 printf("\t     |  PHONEBOOK  |\n");
 printf("\t|-----------------------|\n");
 printf("\t|  1 - INSERT CONTACT   |\n");
 printf("\t|-----------------------|\n");
 printf("\t|  2 - SHOW CONTACT     |\n");
 printf("\t|-----------------------|\n");
 printf("\t|  3 - SEARCH CCONTACT  |\n");
 printf("\t|-----------------------|\n");
 printf("\t|  4 - DELETE CONTACT   |\n");
 printf("\t|-----------------------|\n");
 printf("\t|  5 - ALTER CCONTACT   |\n");
 printf("\t|-----------------------|\n");
 printf("\t|  6 - EXIT             |\n");
 printf("\t|-----------------------|\n");
 printf("\tCHOICE OPTION: ");

 scanf("%d", num);
}

void message1()
{
 printf("\033[2J\033[H");
 printf("\t*-------------------*\n");
 printf("\t|  INSERT CONTACTS  |\n");
 printf("\t*-------------------*\n");
}

void message2()
{
 printf("\033[2J\033[H");
 printf("\t*-----------------*\n");
 printf("\t|  SHOW CONTACTS  |\n");
 printf("\t*-----------------*\n");
}

void message3()
{
 printf("\033[2J\033[H");
 printf("\t*------------------*\n");
 printf("\t|  SEARCH CONTACT  |\n");
 printf("\t*------------------*\n");
}

void message4()
{
 printf("\033[2J\033[H");
 printf("\t*------------------*\n");
 printf("\t|  DELETE CONTACT  |\n");
 printf("\t*------------------*\n");
}

void message5()
{
 printf("\033[2J\033[H");
 printf("\t*-----------------*\n");
 printf("\t|  ALTER CONTACT  |\n");
 printf("\t*-----------------*\n");
}

void message6()
{
 printf("\t*----------------*\n");
 printf("\t|  RECORD FOUND  |\n");
 printf("\t*----------------*\n");
}

void message7()
{
 printf("\033[2J\033[H");
 printf("\t*-------------------*\n");
 printf("\t|  CHANGED CONTACT  |\n");
 printf("\t*-------------------*\n");
}

void message8()
{
 printf("\033[2J\033[H");
 printf("\t*------------------*\n");
 printf("\t|  NAME NOT FOUND  |\n");
 printf("\t*------------------*\n");
}

void getPhone(int *phone)
{
 printf("\tENTER PHONE: ");
 fflush(stdin);
 scanf(" %d", phone);
}

void getName(char str[])
{
 printf("\tENTER NAME: ");
 fflush(stdin);
 scanf(" %[^\n]s", str);
 toUpper(str);
}

void toUpper(char str[])
{
 size_t i;
 for (i = 0; i < strlen(str); i++)
 {
  if (str[i] >= 'a' && str[i] <= 'z')
   {
    str[i]-=32;
   }
 }
}

void option(char op[])
{
 printf("\n\tDo you wan't continue? Enter (Y)-yes (N)-no: ");
 fflush(stdin);
 scanf(" %[^\n]s", op);
}

file: horario.c

#include "phonebook.h"

int main()
{
 int num;
 char op[2];
 char name[100];
 Agenda *raiz = NULL;
 
 do {
     menuPhonebook(&num);

     switch(num)
     {
      case 1:
       do {
           message1();
           getName(name);

           insertContact(&raiz, name);
           printf("\n\tDo you wan't continue? Enter (Y)-yes (N)-no: ");
           scanf("%s", op);

          }while (*op == 's' || *op =='S');
      break;

      case 2:
       message2();
       order(raiz);
      break;

      case 3:
       message3();
       getName(name);
       searchContact(raiz,name);
      break;

      case 4:
       do {
           message4();
           getName(name);
           option(op);

           if(*op == 'n' || *op == 'N')break;

           deleteContact(&raiz,name);break;

          } while(*op == 's' || *op == 'S');           
      break;

      case 5:
       message5();
       getName(name);
       changeContact(&raiz,name);
      break;
     }
    } while(num != 6);
}

Compilation command:

gcc -o horario horario.c -std=c99 -Wall -Wextra -Wpedantic
\$\endgroup\$
5
  • 1
    \$\begingroup\$ 1) fflush(stdin); is undefined behavior. 2) Don't use scanf(). Read a whole line as input with fgets()/getline() and then parse it with strtol()/sscanf() etc. 3) Use islower() from <ctype.h> instead of (str[i] >= 'a' && str[i] <= 'z'). \$\endgroup\$ Commented Jun 9, 2024 at 5:35
  • 1
    \$\begingroup\$ printf("\033[2J\033[H"); : \033[H is unnecessary. Clearing the entire screen will reset the display's row/col to the top-left corner automatically. \$\endgroup\$ Commented Jun 9, 2024 at 6:34
  • 1
    \$\begingroup\$ @Fe2O3 is not going to help them restructure the code. The question is mostly on topic for code review. There are a lot of observations I could make about the code, such as stdio.h should be included by each C file and not included in the phonebook.h file. The only reason we should refer the OP to SO is if the code isn't working as intended. \$\endgroup\$ Commented Jun 9, 2024 at 12:52
  • 1
    \$\begingroup\$ Is the code working as intended? If the output is in the wrong order then the code is not working as intended and the question should be closed. \$\endgroup\$ Commented Jun 9, 2024 at 12:54
  • \$\begingroup\$ scanf(" %[^\n]s" ..., : s is unnecessary. The %[ ] IS the format specifier, complete, for a string, called a scanset. Every character up to the newline, including spaces, is part of the string being stored. 1) Always provide a width specifier, like %10[^\n], to prevent buffer overflow. 2) Always check the returned value from any of the scanf() family of functions. Please study the documentation carefully. (Comment reposted to fix my own copy/paste mistake in the format string... Doh!) \$\endgroup\$ Commented Jun 9, 2024 at 12:58

2 Answers 2

Reset to default
2
\$\begingroup\$

Thank you for telling us that you're writing in the C99 dialect; that's very helpful.

names

 struct Arvore *esq;
 struct Arvore *dir;

I don't understand what's going on here.

We have name and phone fields, yet we didn't want left and right? (I eventually found "esquerda" and "direita", despite google being certain that "certo" was what I wanted. Using an abbrev poses certain challenges and tradeoffs in a single-language codebase. For multi-lingual, they are multiplied.)

I am taking this struct to be an arbor, a Tree. Consider calling it a TreeNode or just a Node. Yes, I know each struct is large, it contains multitudes, subtree after subtree. Most folks would use an identifier like head for a scalar pointing at a LinkedListNode, and an identifier like tree for a scalar pointing at a TreeNode.

languages

Agenda** menor_dir(Agenda *raiz);

More generally, we see a mishmash of languages in this code. Each codebase exists to support certain goals. It must bend and sway to conform to those goals. I recommend you write some of those goals down. And then refactor to use a single language. Multiple people interact with the codebase; they have different language skills and learning objectives. Decide what is the best balance across all that, pick what you feel is the most suitable natural language, and refactor the code to accommodate that.

When starting a new project in future, reflect on this experience and pick a single language from the get go.

meaningful identifier

void message1();
...
void message8();

When I first saw these declarations I thought they were for end-user translations. Then we could also output in French or whatever, using deliberately language agnostic names. But no, they're just constants.

Half a dozen of them seem to not be pulling their own weight. Consider just in-lining them.

Message{6,8} are laudably DRYing up the code, good. Consider renaming to a verb like display_not_found() or print_not_found().

Consider turning both of them into a constexpr or a preprocessor CONSTANT which you pass into printf().

Definitely message7() can be discarded, as it has no callers.

When we get down to the call site, a different pattern suggests itself.

      case 1: ...
           message1();
           ...
      case 2:
       message2();

Maybe you wanted a vector of function pointers together with their corresponding strings? That would be following a strategy pattern.

           printf("\n\tDo you wan't continue? Enter (Y)-yes (N)-no: ");

typo: "Do you want to ..."

Also, despite the instructions, perhaps the initial S is for "si", ignoring "yes"?

Consider replacing this with a call to option(), which currently only case 4 uses.

enum

Consider giving symbolic names to case 3 and friends, such as OPTION_SEARCH.

nit: replace "CCONTACT" with "CONTACT".

C API

In modern C code we pay greater attention to safety than K&R did in 1970. So the standard libc routines have evolved, and we generally expect to see a buffer length specified to defend against buffer overruns. As usual I completely agree with @Harith and @Fe2O3 that it's important to use an appropriate interface and check every returned status. I won't detail the several places this code ignores error status.

internal API

This helper offers a nice API to the caller.

void getPhone(int *phone)
{
 printf("\tENTER PHONE: ");
 fflush(stdin);
 scanf(" %d", phone);
}

(Again, we're ignoring the UB flush behavior, and scanf() details.)

It makes the caller responsible for allocating result storage. Perhaps that's slightly less convenient than having an int getPhone signature, or perhaps we're going for consistency with getName. In any event it's a straightforward API, easily understood, and so simple that it doesn't really need much testing. It performs a UI task and it's a good building block.

OTOH this API is slightly messy:

void searchContact(Agenda *raiz, char str[50])

Most of it is about "searching". But it also does some UI "reporting" when it calls message{6,8}.

Consider making the caller responsible for reporting on search result, so this routine need only worry about coming up with a result. Rather than evaluating a void function for side effects, we would expect to get back a tree node from it.

Similarly for a couple of related routines.

We would like to be able to write down just a single responsbility for each function. We'd rather not say, "it computes this and displays that".

test suite

This codebase would benefit from running automated unit tests. It would be a challenge to do that with the code as written, since core functionality and UI tasks are freely intermingled in the various functions. Separating them would improve the structure, and would also enable automated testing.

A Green test should run silently, without console output. And pausing for interactive input would definitely pose an issue for automated tests.

\$\endgroup\$
2
\$\begingroup\$

Indentation

More than a single space for indentation makes code much easier to read. Four spaces is a very common convention for C.

Nested conditionals

Let's consider one function, with improved indentation:

void insertContact(Agenda **raiz , char str[50])
{
    int phone;

    if (*raiz == NULL)
    {
        *raiz = (Agenda*)malloc(sizeof(Agenda));
   
        strcpy((*raiz)->name, str);
   
        getPhone(&phone);
   
        (*raiz)->phone = phone;
        (*raiz)->esq = NULL;
        (*raiz)->dir = NULL;              
    }
    else
    {
        if (strcasecmp((*raiz)->name,str) > 0)
        {
            insertContact(&(*raiz)->esq, str);                     
        }
        else
        {
            if (strcasecmp((*raiz)->name,str) < 0)
            {
                insertContact(&(*raiz)->dir, str);           
            }
            else
            {
                if (strcmp((*raiz)->name,str) == 0)
                {
                    printf("NAME ALREADY EXISTS!\n");
                }
            }
        }
    }
}

You can use else if to reduce the indentation considerably and make this easier to follow.

We can also get rid of the unnecessary cast on malloc.

void insertContact(Agenda **raiz , char str[50])
{
    int phone;

    if (*raiz == NULL)
    {
        *raiz = malloc(sizeof(Agenda));
   
        strcpy((*raiz)->name, str);
   
        getPhone(&phone);
   
        (*raiz)->phone = phone;
        (*raiz)->esq = NULL;
        (*raiz)->dir = NULL;              
    }
    else if (strcasecmp((*raiz)->name, str) > 0)
    {
        insertContact(&(*raiz)->esq, str);                     
    }
    else if (strcasecmp((*raiz)->name, str) < 0)
    {
        insertContact(&(*raiz)->dir, str);           
    }
    else if (strcmp((*raiz)->name, str) == 0)
    {
        printf("NAME ALREADY EXISTS!\n");
    }
}

However we can note here that we're calling strcasecmp twice, which can lead to duplicated effort. It's also suspect that you're using strcasecmp to determine whether they're less than or greater than, but then strcmp to check for equality.

Let's refactor this a bit.

We'll also print that "NAME ALREADY EXISTS!\n" message to standard error, rather than standard output, so that the user can have more control over it.

void insertContact(Agenda **raiz , char str[50])
{
    int phone;

    if (*raiz == NULL)
    {
        *raiz = malloc(sizeof(Agenda));
   
        strcpy((*raiz)->name, str);
   
        getPhone(&phone);
   
        (*raiz)->phone = phone;
        (*raiz)->esq = NULL;
        (*raiz)->dir = NULL;  
  
        return;            
    }

    int cmp = strcasecmp((*raiz)->name, str);

    if (cmp > 0)
    {
        insertContact(&(*raiz)->esq, str);                     
    }
    else if (cmp < 0)
    {
        insertContact(&(*raiz)->dir, str);           
    }
    else
    {
        fprintf(stderr, "NAME ALREADY EXISTS!\n");
    }
}

Data structures

It's unlikely to make much of a practical difference at the scale you're likely testing, but consider for efficiency's sake using not just a binary search tree, but a balanced binary search tree.

Without the balancing a tree where we insert entries with names "bob", "charlie", "doug", "elias", and "frank" in that order could look like the following, which is essentially just a linked list.

"bob"
    \
    "charlie"
          \
          "doug"
              \
              "elias"
                   \
                   "frank"

Rather than:

          "doug"
          /    \
   "charlie"   "elias"
   /                \
"bob"              "frank"

The former features O(n) lookups, rather than O(log n) lookups like the latter.

You might also consider using a trie.

\$\endgroup\$

You must log in to answer this question.