2
\$\begingroup\$

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;
}
\$\endgroup\$
  • 6
    \$\begingroup\$ Why do you need to eliminate the loop? \$\endgroup\$ – svick Oct 28 '12 at 9:06
1
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ That's a nice answer but; String.Substring method already returns a string, you don't need to call ToString after that. \$\endgroup\$ – Şafak Gür Oct 29 '12 at 9:21
  • \$\begingroup\$ You should a) check that mobileNumber is at least 9 characters long or your Substring call is going to throw an exception and b) use long.TryParse instead of Convert.ToInt54 as that can also throw an exception. \$\endgroup\$ – Trevor Pilley Oct 29 '12 at 11:35
  • 1
    \$\begingroup\$ You should use brackets around one-line fors, ifs, etc. Otherwise indentation errors like the one you did will induce errors/bugs. \$\endgroup\$ – ANeves Oct 29 '12 at 11:38
  • \$\begingroup\$ @ANeves - You're right, I did have an unnecessary indentation in the first example. Fortunately this is C#, not coffescript, the syntax of this is unambiguous; there will never be an error due to indentation. As for single line vs brackets, I prefer to minimize most noise - especially needless vertical whitespace - which can cause important code to be pushed offscreen. There are of course many, many exceptions but when the choice is between using two semantically meaningless lines or not I prefer to go with the later. I find it powerful when combined with small methods and returning early. \$\endgroup\$ – George Mauer Oct 29 '12 at 14:10
  • 1
    \$\begingroup\$ @ANeves - that's fair. Like I said, I disagree with the guidance in a majority of the cases but in this one I can agree that use of braces could have helped readability. Then again you're talking about an intermediate step, not the final solution, I almost never would actually use a for loop much less an if inside of one :P \$\endgroup\$ – George Mauer Oct 29 '12 at 15:32
1
\$\begingroup\$

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?

\$\endgroup\$
0
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ How can i handle different "Number" in the same code(9/8/7/6/5 digits)? \$\endgroup\$ – NewBornDeveloper Oct 28 '12 at 8:26
  • \$\begingroup\$ Edited my answer \$\endgroup\$ – Amiram Korach Oct 28 '12 at 9:10

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.