1
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ What is start for? \$\endgroup\$ – Martin York Mar 10 '12 at 20:39
4
\$\begingroup\$

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
}
\$\endgroup\$
2
\$\begingroup\$
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;
}
\$\endgroup\$
  • \$\begingroup\$ Reverse direction may need a tweek. 1) position/end identical 2) reading one past the end (on first element). \$\endgroup\$ – Martin York Mar 11 '12 at 3:04
  • \$\begingroup\$ @LokiAstari, corrected. But of course anybody who pulls my untested code deserves what they get. \$\endgroup\$ – Winston Ewert Mar 11 '12 at 14:14
  • \$\begingroup\$ "There is not a whole lot of point in storing the length in a variable only to use it once" Yes there is, readability. The result of strlen() will have to be stored somewhere, either in your explicitly declared variable or in an invisible temporary one sneaked into the machine code by the compiler. The machine code will be the same in both cases, so by omitting the variable you haven't optimized a thing. Actually, I bet you only make things worse by calling the function inside both if and else, depending on how CPU branch prediction works. \$\endgroup\$ – Lundin Mar 20 '12 at 15:54
  • \$\begingroup\$ @Lundin, I don't think you gain you readability by doing that. In my view local variables improve readability in three ways: Firstly, by providing an informative name for a value. Secondly, by splitting complex expressions up into multiple pieces. Thirdly, by replacing a non-trivial expression used multiple times. \$\endgroup\$ – Winston Ewert Mar 20 '12 at 16:03
  • \$\begingroup\$ To my mind, the first doesn't apply because the name doesn't tell me anything strlen doesn't. The second doesn't apply because the expression is still pretty simple. The third doesn't apply because the expression being replaced is non-trivial. The code would not be any shorter using a shared variable. \$\endgroup\$ – Winston Ewert Mar 20 '12 at 16:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.