Problem with hash-table insertion function in C

Question

Hey hey,

I am trying to wrap my head around C right now (mostly how pointers operate). I am trying to write this function described below:

/* EnterName enter a Name into a symbol table. Returns a boolean indicating whether an entry for Name was already listed in the table. Passes back an argument containing an entry reference for the name.

Anyway, here is the code I have written and I am not sure how to test it at the moment. Wondering if someone could look over it and let me know if I am doing this right.

Thanks in advance.

Code::

bool EnterName(struct SymTab *ATable,
              const char *Name,
              struct SymEntry * *AnEntry)
{
              char name = *Name;
              unsigned hashval = hash (&name);
              struct SymEntry *ptr;

          ptr = ATable->Contents[hashval];

          while(ptr != NULL)
          {
                    if(strcmp(ptr->Name, &name)) 
                    {
                                 AnEntry = &ptr;
                                 return true;
                    } 
          }

          ptr = malloc(sizeof(struct SymEntry));
          ptr->Name = &name;
          AnEntry = &ptr;
          return false;

}

Answer 1

Does it run and produce the results you expect?

There's no replacement for compiling code, running it with test data, comparing results to expected values, debugging, etc, when learning a new language. Showing untested code to others and asking if it's OK isn't the right approach.

Even the first two lines of the function don't make sense:

   char name = *Name;
   unsigned hashval = hash(&name);

You've just taken the first character of Name into the name variable, and then try to hash its address.

Now, the loop:

          while(ptr != NULL)
          {
                    if(strcmp(ptr->Name, &name)) 
                    {
                                 AnEntry = &ptr;
                                 return true;
                    } 
          }

Doesn't make sense either, since you're not advancing ptr anywhere - it's an infinite loop.

That said, there seems to be a step in the right direction in your code. You just have to get your types straight and fix all the rough corners. I suggest starting with small pieces - see that they compile and run, then proceed assembling into larger pieces.

Answer 2

I can spot at least one error. You're only hashing the first letter of each name...
name = a single chatacter (a copy) hash(&name), hashes the copy of the single character. You want hash(Name), with a capital, so that you give has the beginning of the sting to hash, so it can hash the whole thing.

Answer 3

char name = *Name;

This takes the first character of Name and stores it in name (The string Hello would be truncated to a single H). You certainly want name to be char* (pointer to a array of characters) instead of a single character.

strcmp(a,b) returns 0 if a matches b. I think it should be

if(!strcmp(ptr->Name, name)) 

Also, your loop might run forever. if the first comparison fails (provided you add the !), the loop would continue forever since ptr's value will never change.

There's much wrong in your code. You HAVE TO test it. Take a compiler, compile it, run it. Programming without testing is not possible.

Answer 4

This

AnEntry = &ptr;

isn't doing what you want it to do. It is changing the value of the local variable AnEntry without changing the value in the calling routine.

You wanted

*AnEntry = ptr;

And there are several other bugs, and likely mistakes, but they seem to be reasonably well covered by others...