Finding one string located in another

Question

Can anyone help me improve my instr() function?

int myInstr(wchar_t *str, wchar_t c, int start, int dir){
    int pos = 0, result = 0, len;
    wchar_t *p1;

    //Left
    if(dir == 0)
    {
        p1 = str;
        len = lstrlen(str);
        while(pos < len)
        { 
            if(*p1 == c) 
            {
                result = pos;
                return result;
            }
            p1++;
            pos++;
        }
    }
    else
    {
      //Right
      pos = len = lstrlen(str);
      p1 = str;
      p1 += len;
      while(pos >= 0)
      {
          if(*p1 == c) 
          {
              result = pos;
              return result;
          }
          p1--;
          pos--;
      }
    }
    return 0;
}

Answer 1

Here are my comments:

Functionality:

• Remove the superfluous start parameter.
• Remove all needlessly complex (and slow) pointer arithmetic.
• Use the standard wcslen() function from wchar.h
• Position 0 is a valid array index, replace it with an invalid one if value not found.

Performance:

• Only call the strlen function once.
• Minimize the code inside the if/else statement.

Readability and style:

• str isn't modified by the function, so make it const (const correctness).
• Rewrite the loop to a conventional one.
• Only return once from a function.
• Replace all "magic numbers" with constants or enums.

-

#include <stddef.h>
#include <wchar.h>
#include <stdio.h>

typedef enum
{
  DIR_POS,
  DIR_NEG 
} dir_t;


#define NOT_FOUND -1


int myInstr (const wchar_t*  str,
             wchar_t         c,
             dir_t           dir)
{
  int start;
  int end;
  int step;
  int length;
  int i;
  int result = NOT_FOUND;

  length = wcslen(str);

  if(dir == DIR_POS)
  {
    start = 0;
    end   = length;
    step  = 1;
  }
  else /* if (dir == DIR_NEG) */
  {
    start = length - 1;
    end   = -1;
    step  = -1;
  }

  for(i=start; i!=end; i+=step)
  {
    if(str[i] == c)
    {
      result = i;
      break;
    }
  }

  return result;
}


int main()
{
  wchar_t str [] = L"1234567890";

  printf("%d\n", myInstr(str, '3', DIR_POS)); // prints 2
  printf("%d\n", myInstr(str, '6', DIR_NEG)); // prints 5
  printf("%d\n", myInstr(str, 'X', DIR_POS)); // prints -1
}

Answer 2

int myInstr(wchar_t *str, wchar_t c, int start, int dir){
int pos = 0, result = 0, len;
wchar_t *p1;

Names like p1 are best avoided. Its hard to guess what p1 might be for.

//Left
if(dir == 0)
{
    p1 = str;
    len = lstrlen(str);
    while(pos < len)

There is not a whole lot of point in storing the length in a variable only to use it once

    { 
        if(*p1 == c) 
        {
            result = pos;
            return result;

Just return pos

        }
        p1++;
        pos++;

You are keeping track of the same information twice. If you can just keep track of it once.

    }
}
else
{
    //Right
    pos = len = lstrlen(str);
    p1 = str;
    p1 += len;
    while(pos >= 0)
    {
        if(*p1 == c) 
        {
            result = pos;
            return result;
        }
        p1--;
        pos--;
    }

This is very similiar to the other cases. See if you can't combine the cases.

}
return 0;

This seems a poor choice to indicate failure to find. What if it was found at position 0?

Here is my untested version of your code:

int myInstr(wchar_t *str, wchar_t c, int start, int dir){
    int step;
    wchar_t * position;
    wchar_t * end;

    if(dir == 0)
    {
        position = str;
        end = str + lstrlen(str);
        step = 1;
    }
    else
    {
        position = str + lstrlen(str) - 1;
        end = str;
        step = -1;
    }

    while(position != end)
    {
        if(*position == c)
        {
            return position - str;
        }
        position += step;
    }

    return -1;
}