How Would You Refactor this Code? #1

It's amazing how many different ways there are to accomplish the same task in code.  Talk to 2 developers and you'll almost always get two opinions.  That's part of what keeps it fun (and potentially why that guy in the other cube is constantly annoying you :-)).  In a previous post I wrote, there were several speed enhancement suggestions that were really good so I decided to start a "How Would You Refactor this Code" blog series to get opinions and see different approaches to coding. 

The first sample I wanted to post does a fairly simple string check to see if a string starts with a set of characters.  The value of the string being compared is variable.  There are multiple ways to do this that come to mind ranging from using a single string along with Contains(), regex, character by character comparisons, etc.  After a whole 10 seconds of thought I decided to go with a Lambda expression because I'm using Lambdas and LINQ a lot lately (and it's kind of fun).  It's not "the way" or the "best way" which is the point of this exercise.  So, how would you refactor this code?

string name = "mc_gross_1";  //The prefix and number are variable
string[] cartNames = { "item_number_", "item_name_", "mc_gross_", "quantity_" };
if (cartNames.Where(p => name.StartsWith(p)).Count() > 0)
{
    //process name
}


If you have refactoring ideas for this code or other "How Would You Refactor this Code" suggestions for future posts please add a comment. 

Code Refactoring Suggestions:

Here are some of the code refactoring suggestions to this point (read the comments for additional thoughts).  Great stuff.  If you have other suggestions keep them coming.

Suggestion # 1:

if (cartNames.Any(p => name.StartsWith(p)))
{
    //process name
}


Rationale for this refactoring:

You need only to know if the name is found in the array.  In this case the extension method Any() will do the best job for you.

The solution with Where() and Count() is not good. With Any() at the first match you will have the result and with Where() you will traverse the entire collection and then Count() the items.

Note 1: Count() is an extension method not a property so it has a complexity of O(n).

Note 2: Where() will create a collection to store matches(allocating memory). Any() will return a simple bool.

More semantically correct, because you're not really interesting in counting - just existence - it also has the added bonus of being more succinct.

Fabrice Marguirie was one of several people that suggested this refactoring.  I just purchased the EBook version of his LINQ in Action book and have found it to be very good so far.

Suggestion #2:

//C# 3.0
if (Array.Exists(cartNames, p => name.StartsWith(p)))
{
    // Process name
}

//C# 2.0
if (Array.Exists(cartNames, delegate(string p)
{
    return name.StartsWith(p);
}))
{
    // Process name
}


Rationale for this refactoring:

The most obvious suggestion is to use the .NET 2.0 "Array.Exists" method, rather than a .NET 3.5 LINQ query. Aside from the problem that many PCs don't have .NET 3.5 installed yet - and you can't install it on Windows 2000 - the Exists method can be more efficient than a count, as it will stop after the first match, rather than continuing to the end of the list.

Suggestion #3:

string name = "mc_gross_1";  //The prefix and number are variable
if (new Regex(@"(item_number_|item_name_|mc_gross_|quantity_)\d").Matches(name).Count > 0)
{
    // Process name
}

List<string> cartNames = new List<string>(new string[] { "item_number_", "item_name_", "mc_gross_", "quantity_" });
if (cartNames.Contains(name.Substring(0, name.LastIndexOf("_") + 1)))
{
    // Process name
}


Rationale for this refactoring:

JV (and colleagues) focused on how to solve this coding issue without using LINQ.

Suggestion #4:

using System;
using System.Collections;
using System.Collections.Generic;
using System.Globalization;

public sealed class StringPrefixComparer : IComparer<string>, IComparer
{

    private readonly CompareInfo _compareInfo;
    private readonly CompareOptions _options;

    private StringPrefixComparer(CompareInfo compareInfo, CompareOptions options)
    {
        _compareInfo = compareInfo;
        _options = options;
    }

    public static StringPrefixComparer Ordinal
    {
        get
        {
            return new StringPrefixComparer(
                CultureInfo.InvariantCulture.CompareInfo,
                CompareOptions.Ordinal);
        }
    }

    public int Compare(string x, string y)
    {
        if (string.IsNullOrEmpty(x)) return (string.IsNullOrEmpty(y)) ? 0 : -1;
        if (string.IsNullOrEmpty(y)) return 1;
        int length = (x.Length > y.Length) ? y.Length : x.Length;
        return _compareInfo.Compare(x, 0, length, y, 0, length, _options);
    }

    int IComparer.Compare(object x, object y)
    {
        return this.Compare(x as string, y as string);
    }

}

static class Program
{

    static bool Exists<T>(this T[] list, T item, IComparer<T> comparer)
    {
        return 0 <= Array.BinarySearch(list, item, comparer);
    }

    static void Main()
    {

        string[] cartNames = { "item_name_", "item_number_", "mc_gross_", "quantity_" };
        Array.Sort(cartNames, StringComparer.Ordinal);
        string name = "mc_gross_1";
        if (cartNames.Exists(name, StringPrefixComparer.Ordinal))
        {
            // Process name
        }
    }
}

Rationale for this refactoring:

Assuming that the array of prefixes is constant, ensure that the array is sorted, and use the Array.BinarySearch() method with a custom IComparer<string> implementation.  Uses the new C# 3.0 extension method capability and does a clever trick in the Compare() method that avoids having to explicitly worry about stripping off the numeric character on the end of the name variable.   Thanks to Richard for taking the time to post this one.

comments powered by Disqus

6 Comments

  • In my opinion this is what RegEx was made for. So I would solve it with regex I guess.

  • I think cartNames.Any(p => name.StartsWith(p)) is more semantically correct, because you're not really interesting in counting - just existence - it also has the added bonus of being more succinct.

  • That code is pretty concise as it stands.

    My instinct would be to look beyond that section. Doing string comparison to process what look like fundamental business properties has a bad smell, and I'd be looking for ways to strongly type them.

  • So me and my collegeau had a nice discussion about this. Mainly focused on how to solve this without using linq. And we came up with these two solutions regex and plain old string comparision:

    string name = "mc_gross_1"; //The prefix and number are variable

    if (new Regex(@"(item_number_|item_name_|mc_gross_|quantity_)\d").Matches(name).Count > 0)
    {
    Console.WriteLine("process name");
    }

    List cartNames = new List(new string[] { "item_number_", "item_name_", "mc_gross_", "quantity_" });

    if(cartNames.Contains(name.Substring(0, name.LastIndexOf("_")+1)))
    {
    Console.WriteLine("process name");
    }

    But as you said (and many others also have shown here), we got millions of roads nowadays heading to Rome :)

  • Well, my personal wish for Regex this year (yeah, I ask .NET objects for presents, not Santa, but that's me) was to be able to match multiple expressions in the same time. In that case, your code would have been: if (Regex.IsAnyMatch(cartNames)) process. The rationale of it being that it would abandon matches in mid pattern if another one is successful at that point and the compilation of that regex could be cached or even stored in a DLL.

  • Another alternative:

    Assuming that the array of prefixes is constant, ensure that the array is sorted, and use the Array.BinarySearch method with a custom IComparer implementation.

    using System;
    using System.Collections;
    using System.Collections.Generic;
    using System.Globalization;

    public sealed class StringPrefixComparer : IComparer, IComparer
    {
    private readonly CompareInfo _compareInfo;
    private readonly CompareOptions _options;

    private StringPrefixComparer(CompareInfo compareInfo, CompareOptions options)
    {
    _compareInfo = compareInfo;
    _options = options;
    }

    public static StringPrefixComparer Ordinal
    {
    get
    {
    return new StringPrefixComparer(
    CultureInfo.InvariantCulture.CompareInfo,
    CompareOptions.Ordinal);
    }
    }

    public int Compare(string x, string y)
    {
    if (string.IsNullOrEmpty(x)) return (string.IsNullOrEmpty(y)) ? 0 : -1;
    if (string.IsNullOrEmpty(y)) return 1;

    int length = (x.Length > y.Length) ? y.Length : x.Length;
    return _compareInfo.Compare(x, 0, length, y, 0, length, _options);
    }

    int IComparer.Compare(object x, object y)
    {
    return this.Compare(x as string, y as string);
    }
    }

    static class Program
    {
    static bool Exists(this T[] list, T item, IComparer comparer)
    {
    return 0 <= Array.BinarySearch(list, item, comparer);
    }

    static void Main()
    {
    string[] cartNames = { "item_name_", "item_number_", "mc_gross_", "quantity_" };
    Array.Sort(cartNames, StringComparer.Ordinal);

    string name = "mc_gross_1";
    if (cartNames.Exists(name, StringPrefixComparer.Ordinal))
    {
    // Process name
    }
    }
    }

Comments have been disabled for this content.