Testable MVC Models - Part 1: The Naive Approach | Brevity is the Soul of Maintainability

If You're Looping, Look for a Better Way

by Larry Spencer Friday, April 5, 2013 3:43 PM

Today, it's more from the Code Review front. This is really the same point I made last time: Brevity is the Soul of Maintainability.

I just saw the following code in a review. It's part of a method whose input is a string called line. What do you think it does?

int verticalBarCount = 0;
string tempLine = line;
int off = tempLine.IndexOf('|');
while (off != -1)
{
    verticalBarCount++;
    tempLine = tempLine.Substring(off + 1, tempLine.Length - (off + 1));
    off = tempLine.IndexOf('|');
}

After only a little head-scratching, and aided by the well-named variable, verticalBarCount, you probably surmised that the code counts the number of vertical bars in the input line. That's correct, and the code is correct. All well and good.

However, isn't this equally correct code a whole lot better?

int verticalBarCount = Regex.Matches(line, @"\|").Count;

The moral I'd like to draw from this vignette is: If you're looping, look for a better way. That's especially true if the loop is doing something low-level, like counting characters.

7bb6a32c-b039-4b12-a49d-46f5e2b4e151|0|.0|27604f05-86ad-47ef-9e05-950bb762570c

Tags: Best Practices, Code Review

All | General

Comments (8) -

4/8/2013 4:06:00 PM #

Sometimes (probably fairly often) a loop is the right way to code something.
In this case, the loop could be made much simpler, just by using the two parameter overload of IndexOf and bypassing the creation of the substrings.
For readability, this should probably be refactored into a (extension?) method that takes a string and a char, and returns the count.
Recommending using Regex, which is computationally expensive by comparison, hardly seems like good advice for this.
I think even using LINQ would be simpler:
int verticalBarCount = line.Count(c => c == '|');

Matt Heffron

4/9/2013 1:35:33 PM #

@Matt Heffron - Yes, the "before" example in my post is extreme and the loop could have been improved even as a loop. However, we do agree that -- in this case -- there are at least two non-looping solutions that are better.

You are absolutely right about LINQ being faster. I wrote a little program that finds bars in a line that has 9 of them, 100,000 times, with Regex and then with LINQ. Regex took 295 milliseconds, or about 3 millionths of a second per line. LINQ took 116 milliseconds, or about 1 millionth of a second per line. Whether that's enough difference to worry about would depend on the application.

Another consideration would be which version is easier to read. Regexes are notoriously difficult to read, so for any application other than very simple ones like my example, I would go with LINQ as you suggest.

Larry Spencer

4/9/2013 1:37:26 PM #

@Matt Heffron again:

P.S. - Maybe I should write another post called If You're Using Regex, Look for a Better Way - haha.

Larry Spencer

4/9/2013 2:20:06 PM #

Actually, the loop form is much faster than even Linq:
10,000,000 iterations:
Linq Count is 3.388 seconds (0.3388 microseconds each)
loop in method is 0.928 seconds (0.0928 microseconds each)
    static int CountChar(string line, char test)
    {
      int count = 0;
      int offset = line.IndexOf(test);
      while (offset != -1)
      {
        count++;
        offset = line.IndexOf(test, offset+1);
      }
      return count;
    }

Matt Heffron

4/11/2013 8:53:47 AM #

...and assembly language would be faster than all of them, but that doesn't mean we want to use assembly language.

Larry Spencer

4/11/2013 1:37:50 PM #

Yes, but the loop is straightforward, and by encapsulating it in a method, it can be easily understood where used.

Matt Heffron

4/12/2013 11:08:43 AM #

So, if I find a better way, should I then repeat the process?

Joe

4/12/2013 12:27:32 PM #

"Some people, when confronted with a problem, think “I know, I'll use regular expressions.” Now they have two problems."

(http://regex.info/blog/2006-09-15/247)

Fascinated with Software

The wonders of C#, JavaScript, LINQ, the Entity Framework and all things .NET

If You're Looping, Look for a Better Way

Comments (8) -

Add comment

About the Author

RecentPosts

Fascinated with Software

The wonders of C#, JavaScript, LINQ, the Entity Framework and all things .NET

If You're Looping, Look for a Better Way

Comments (8) -

Add comment

About the Author

RecentPosts

Tag cloud