9
\$\begingroup\$

I have a function that returns the index of the first alphabet letter found in a string, or -1 if no letter is found. It seems like there should be a way to do this with extension methods instead of looping, but I can't think of one:

int findFirstLetter(string str)
{
 for(int ctr=0;ctr<str.Length;ctr++)
 {
  if (Char.IsLetter(str[ctr]))
  {
   return ctr;
  }
 }
 return -1;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ return str.ToList().FindIndex(c => Char.IsLetter(c)); \$\endgroup\$ – ChrisW Mar 7 '14 at 23:56
  • \$\begingroup\$ I'd like to vote-to-close this question, if you don't mind: it's more of a "code golf" question than a code review. \$\endgroup\$ – ChrisW Mar 7 '14 at 23:57
  • 1
    \$\begingroup\$ @ChrisW, does that mean I should post questions like this to Code Golf? \$\endgroup\$ – sigil Mar 8 '14 at 0:01
  • 2
    \$\begingroup\$ I don't know but I don't think it's on-topic there: it's not a puzzle or game. Perhaps it is on-topic here, I don't know: meta.codereview.stackexchange.com/a/711/34757 suggests "no" but meta.codereview.stackexchange.com/q/1583/34757 suggests "yes". \$\endgroup\$ – ChrisW Mar 8 '14 at 0:07
  • 1
    \$\begingroup\$ @nhgrif - I think readability is subjective. If you are new to Linq ChrisW's solution is a little mind boggling. But after spending time with Linq, you start to "think in Linq." I actually find his answer far more readable: take a string, make a list out of it, find the index of something in that list, that something is a letter. \$\endgroup\$ – ThisGuy Mar 8 '14 at 7:08
9
\$\begingroup\$

I have no issue with the loop. It is about as fast as it can be. I would recommend renaming the method to match the other IndexOf methods.... something like IndexOfAlphaChar.

There is no reason to worry about this code. Move on to bigger problems.

\$\endgroup\$
  • \$\begingroup\$ Maybe nitpicking but I do have an issue with the loop! It could be about 25% faster by storing str.Length in a temporary variable strLen and then using this in the loop condition ctr<strLen. No need to keep re-evaluating str.Length if the string length never changes. \$\endgroup\$ – wardies Feb 18 '16 at 16:59
6
\$\begingroup\$

I think that the biggest problem with the code you posted is not the structure, but variable naming and other stylistic issues:

  1. It took me a while to realize what ctr meant (control?). Don't shorten variable names like this, write them in full if you can.
  2. Opinions about the right indentation style vary wildly, but I think that one space is too little. The common values are 2 or 4 spaces or a single tab (\t).
  3. Use spaces more often to make the structure of your code clearer:

    for (int ctr = 0; ctr < str.Length; ctr++)
    
  4. For types that can be written as a keyword, I always write them that way. That means I would write char.IsLetter (lowercase c).

\$\endgroup\$
  • \$\begingroup\$ I agree with you about the tabs and spacing; when I use Ctrl+K,D in Visual Studio, the text gets formatted in accordance with your suggestions. With regard to Char.IsLetter, Char is a primitive in C# that has a static method IsLetter. So it has to be a capital C because C# is case-sensitive. \$\endgroup\$ – sigil Mar 10 '14 at 16:46
  • 2
    \$\begingroup\$ @sigil Yes, C# is case-sensitive, but the type char is exactly the same as System.Char. So, both versions will work the same here (unlike e.g. in Java, where char and Character are different types). \$\endgroup\$ – svick Mar 10 '14 at 16:50
  • \$\begingroup\$ I didn't know that, thanks for pointing that out. \$\endgroup\$ – sigil Mar 10 '14 at 16:53

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.