Checking mobile SIMs

Question

Can someone improve this code? I need to eliminate the for loop.

public bool CheckMobileSim(List<Mobile_Range> numberRange, string MobileNumber)
{
    bool SimType = new bool();
    string NineDigits = MobileNumber.Substring(0, 9).ToString();
    long Number = Convert.ToInt64(NineDigits);
    if (numberRange != null)
    {
        if (numberRange.Count > 0)
        {
            for (int i = 0; i < numberRange.Count; i++)
            {
                if ((Number >= numberRange[i].RangeStart && Number < numberRange[i].RangeEnd))
                {
                    SimType = true;
                    break;
                }

            }
        }
    }

    return SimType;
}

Answer 1

Refactoring step 1, remove unnecessary variables and braces and return early

public bool CheckMobileSim(List<Mobile_Range> numberRange, string MobileNumber)
{
    if(numberRange == null)
        return;
    string NineDigits = MobileNumber.Substring(0, 9).ToString();
    long Number = Convert.ToInt64(NineDigits);
    for (int i = 0; i < numberRange.Count; i++)
        if ((Number >= numberRange[i].RangeStart && Number < numberRange[i].RangeEnd))
            return true;

    return false;
}

I realize that breaks with the "single point of return" wisdom but I'm not a big fan. When you have small functions like this one, it doesn't make all that much sense.

Refactoring step 2, with that cleaned up it's easy to see how this fits into a simple LINQ query

public bool CheckMobileSim(IEnumerable<Mobile_Range> numberRange, string mobileNumber)
{
    if(numberRange == null || String.IsNullOrWhitespace(mobleNumber))
        return false;

    var nineDigits = mobileNumber.Substring(0, 9);
    var number = Convert.ToInt64(nineDigits);

    return numberRange.Any(n => number >= n.RangeStart && number < n.RangeEnd);
}

Since this is a public method I added some checks and downcast List to IEnumerable, which is a looser contract and all you really need here.

By the way, .Net naming conventions are pascalCase for private and local variables CamelCase for public and protected. It's rare to use underscores.

Answer 2

You can improve it like this:
(I talked about why I did the changes I made, below the code)

public bool CheckMobileSim(IEnumerable<MobileRange> numberRange, string mobileNumber)
{
    // Sanity checks: You can return false instead of throwing exceptions if you'd like to.
    if (numberRange == null)
        throw new ArgumentNullException("numberRange");

    if (mobileNumber == null)
        throw new ArgumentNullException("mobileNumber");

    // More sanity checks: You need to be sure that mobileNumber has at least nine
    // characters that can be converted to a 64 bit integer.
    if (mobileNumber.Length < 9)
        return false;

    long number;
    if (!long.TryParse(mobileNumber.Substring(0,9), out number))
        return false;

    // You can avoid writing the loop yourself and use LINQ instead.
    return numberRange.Any(r => number >= r.RangeStart && number < r.RangeEnd);
}

Although Enumerable.Any extension method I have used above will essentially do the same thing with:

foreach (var item in numberRange)
    if (number >= item.RangeStart && number < item.RangeEnd)
        return true;

return false;

Why did I change the type of numberRange?

• Because an IList<T> is not needed unless you want to manipulate the collection. If all you want to do is to enumerate it, then an IEnumerable<T> would be sufficient. With this signature, you can pass your List<T> instance or any other IEnumerable<T> instance (an array or a HashSet<T> for example) to this method.

Why did I threw exceptions in first two sanity checks and returned false in others?

• Because most of the time a null parameter means there is something wrong in your code whereas an empty collection does not. I suggest passing the mobileNumber as Int64, avoiding its conversion in this method entirely.

Why did I change your variable names?

• For readability:
  See Capitalization Styles section in MSDN's Naming Guidelines for Class Library Developers page.

Answer 3

public bool CheckMobileSim(List<Mobile_Range> numberRange, string MobileNumber)
{
    var digits = new List<long>();
    for (int i = 5; i < 10; i++)
        digits.Add(Convert.ToInt64(MobileNumber.Substring(0, i).ToString()));

    return numberRange != null &&
        numberRange.Any(nr => digits.Any(n => n >= nr.RangeStart && n < nr.RangeEnd));
}

This way you don't need a loop but there is a loop behind.