CODEWITHSUNDEEP
c++visual-studio-2010
shiran bar
shiran bar

Reputation: 29

stricmp doesn't work

i'm having a problem using stricmp in a specific function, in other functions it works perfectly, except this one. the problem is that even if it compares the same string (char*) it doesn't return 0. what might be the problem? (sorry for the mess, i'll try formatting it) that's the code:

Employee* CityCouncil::FindEmp(list<Employee*> *lst, char* id)  
{  
 bool continue1 = true;  
 Employee* tmp= NULL;  
 char* tmpId = NULL, *tmpId2 = NULL;  
 list<Employee*>::iterator iter = lst->begin();  
 if ((id == NULL) || (lst->empty()==true))  
  return NULL;  
 while ((iter != lst->end()) && (continue1)){  
  tmp = (Employee*)(*iter);  
  tmpId = (*tmp).GetId();  
  if(tmpId != NULL)  
  {  
      if(stricmp(tmpId,id) == 0)  
       continue1 = false;  
  }  
  if (continue1 == true)  
  iter++;  
 }  
 if (iter == lst->end())
     return NULL;
 return (Employee*)(*iter);
}

Upvotes: 1

Views: 3178

Answers (2)

Josh Kelley
Josh Kelley

Reputation: 58362

Your code looks correct; maybe you're not passing in the strings that you think you are, or maybe memory is getting corrupted somehow?

However, your function could be written much more cleanly.

// Why is this a method off of CityCouncil?  As written, it
// doesn't use any CityCouncil members.
//
// Also, why not pass lst by reference?  Like this:
//    Employee* CityCoucil::FindEmp(list<Employee*>& lst, char *id) { ...
//
// Also, const correctness is a good idea, but that's more complicated.
// Start with this:
//    Employee* CityCoucil::FindEmp(const list<Employee*>& lst, const char *id) { ...
//
// Also, it's easy to leak memory when using a list of pointers, but that's
// another topic.
Employee* CityCouncil::FindEmp(list<Employee*> *lst, char* id)  
{
    // No need for continue1; we'll use an early return instead;
    // No need for tmp, tmpId, tmpId2; just call methods directly
    // off of iter.

    if (id == NULL || lst->empty())
        return NULL;

    // You should NOT do a C-style "(Employee*)" cast on *iter.
    // C-style casts are generally to be avoided, and in this case, 
    // it shouldn't be necessary.

    // Use a for loop to simplify your assigning iter and incrementing it.
    for (list<Employee*>::iterator iter = lst->begin(); iter != list->end(); iter++) {
        if ((*iter)->GetId()) {
            if (stricmp((*iter)->GetId(), id) == 0) {
                return *iter;
            }
        }
    }  
    return NULL;
}

Upvotes: 1

AndiDog
AndiDog

Reputation: 70158

Never blame a function that belongs to the C library. stricmp surely works as expected, meaning the strings are really different. There must be something wrong with the logic in this function - you should use printf statements to find out where and why the strings differ.

EDIT: I put together a simple test program. This works for me:

#include <stdio.h>
#include <list>
using namespace std;

// Dummy
class Employee
{
    public:
        Employee(const char *n){ id = strdup(n); }
        char *id;
        char *GetId() { return this->id; }
};

Employee* FindEmp(list<Employee*> *lst, char* id)
{
    bool continue1 = true;
    Employee *tmp = NULL;
    char* tmpId = NULL;

    list<Employee*>::iterator iter = lst->begin();
    if(id == NULL || lst->empty())
        return NULL;

    while(iter != lst->end() && continue1)
    {
        tmp = (Employee*)(*iter);
        tmpId = (*tmp).GetId();
        if(tmpId != NULL)
        {
            if(stricmp(tmpId,id) == 0)
                continue1 = false;
        }
        if(continue1 == true)
            iter++;
    }

    if(iter == lst->end())
        return NULL;

    return (Employee*)(*iter);
}


int main(int argc, char **argv)
{
    list<Employee*> l;

    l.push_back(new Employee("Dave"));
    l.push_back(new Employee("Andy"));
    l.push_back(new Employee("Snoopie"));

    printf("%s found\n", FindEmp(&l, "dave")->GetId());
    printf("%s found\n", FindEmp(&l, "andy")->GetId());
    printf("%s found\n", FindEmp(&l, "SnoOpiE")->GetId());

    return 0;
}

Note that I used the function you provided. Again, there is nothing wrong with stricmp, the problem must be in your code.

Upvotes: 5

Related Questions