alphabetic order function doesnot work why?

Question

 void addContact(contactEntry** phoneBook, char* name, char* surname)
 {   
   /*look*/    
   contactEntry *tempEntry=malloc(sizeof(contactEntry)); /*look*/
   NEXT(tempEntry)=NULL;  PHONELIST(tempEntry)=NULL;   
   contactEntry *newContact=malloc(sizeof(contactEntry));

   curEntry = (*phoneBook); 
   NEXT(newContact)=NULL;
   PHONELIST(newContact)=NULL;

   /* look */
   while(!(strcmp(name,NAME(NEXT(surEntry)))>0)) /*look*/
   {  
      curEntry=NEXT(curEntry);
   }
   tempEntry=NEXT(curEntry);      
   newEntry=NEXT(curEntry);
   tempEntry=NEXT(curEntry);
   free(tempEntry);
 }

A new contact should be located in the proper location which ensures the alphabetical ordering of the contacts. I tried to write that function. Why doesn't that function work?(if you want to solve, just look where I show because I think error is located in .. line

Answer 1

Well, hmm, you really should tell us what the definitions of contractEntry, NEXT, NAME, and PHONELIST are. But let me assume for the moment that they're something like "struct {char *name, char *phonelist, contactEntry *next} contactEntry" and "#define NAME(e) e->name", "#define NEXT(e) e->next" etc. I am also assuming that the contact entries are a linked list.

With those assumptions, I see 3 obvious problems with your code:

1. In the WHILE statement you refer to "surEntry" when you surely mean "curEntry". Perhaps this is just a typo in copying the code to the website.

2. More seriously, you create two objects, tempEntry and newEntry, that are never added to the linked list, no pointer to them is ever saved, and then you exit the function, losing the pointers. I don't see how you ever add anything to the list.

3. You never copy the name into either of these new objects.

I think what you want to say is something more like:

void addContact(contactEntry** phoneBook, char* name, char* surname) 
{    
  contactEntry *newContact=malloc(sizeof(contactEntry)); 
  NAME(newContact)=name;
  // or maybe strcpy(NAME(newContact),name), depending on how your structure is defined
  NEXT(newContact)=NULL; 
  PHONELIST(newContact)=NULL; 

  curEntry = (*phoneBook);  
  while(!(strcmp(name,NAME(NEXT(surEntry)))>0)) /*look*/ 
  {   
    curEntry=NEXT(curEntry); 
  }
  NEXT(newEntry)=NEXT(curEntry);
  NEXT(curEntry)=newEntry;
} 

Your code seems to imply that you have a sentinel as the first entry (i.e. a dummy entry, so the list is never empty). I'm running with that assumption. I'm also assuming that initially the sentinel points to itself. If this is not true, then you have to deal with special cases when adding the first entry and when adding an item that comes after the last entry.

Answer 2

You may want to split your phone list into at least two pieces: the linked list and everything else. By extracting (refactoring) your linked list from your phone list, you can use the list for other assignments (tasks).

Also, with linked list code, macros and #define do not work well. Using #define to reduce your typing is actually a hindrance to developing your code. In your example (as posted), the NEXT is difficult to debug, especially if there is an error in it.

---

Here is some code to help you in your quest:

Data Structures

struct Single_Link
{
  void *               p_node_data;  // Points to the user's data
  struct Single_Link * p_next;
};

/* Optionally for doubly linked list: */
struct Double_Link
{
  void *               p_node_data;
  struct Double_Link * p_next;
  struct Double_Link * p_previous;
};

struct Single_List_Header
{
  struct Single_Link *  p_first_node;
  struct Single_Link *  p_last_node;    // Points to last node for easy appending.
  unsigned int          nodes_in_list;  // For quick access to size of the list.
};

Generic Functions

void Linked_List_Initialize(struct Single_List_Header * p_header)
{
  if (p_header)
  {
    p_header->p_first_node = 0;
    p_header->p_last_node = 0;
    nodes_in_list = 0;
  }
  return;
}

void Linked_List_Insert_No_Sort(struct Single_List_Header * p_header,
                                void *                      p_node_data)
{
  if (p_header)
  {
    // Create new node
    struct Single_Link p_new_node = malloc(sizeof(struct Single_Link));
    if (p_new_node)
    {
      // Initialize the new node.
      p_new_node->p_next = 0;
      p_new_node->p_node_data = p_node_data;

      // Insert node to beginning of list
      if (p_header->p_last_node == 0)
      {
         p_header->p_last_node = p_new_node;
      }
      p_new_node->p_next = p_header->p_first_node;
      p_header->p_first_node = p_new_node;
      ++(p_header->nodes_in_list);
    }
  }
  return;
}

typedef void (*Callback_Function_Ptr)(void * p_node_data);

void Linked_List_Traverse(struct Single_List_Header * p_header,
                          Callback_Function_Ptr       user_function)
{
  struct Single_Link *  p_node = 0;
  if (!p_header || ! user_function)
  {
     return;
  }
  if ((p_header->nodes_in_list == 0) || (!p_header->p_first_node))
  {
     return;
  }
  p_node = p_header->p_first_node;
  while (p_node)
  {
     user_function(p_node->p_node_data);
     p_node = p_node->p_next;
  }
  return;
}

}

Summary

The data structures and functions above have no reference to specialized or specific data. The data in a node is referenced by a pointer, more succinctly, a pointer to void. This allows the Linked List data structure to be used by other entities, such as a Phone List, a List of Songs, etc.

By factoring out the specialization, the development is focused on the Linked List. No worrying about User I/O or data integrity. Also, once this works, it will not have to be debugged for your other assignments (projects). Just place it in its own translation unit (or compile to a library). No more wasting time developing a new Linked List.

BTW, another time saving tip is to search Stack Overflow or the web before posting any questions. Most likely, somebody else has encountered the same issue or something close. Also, there may be code out there that has already been tested, so you don't have to develop new code.