8
\$\begingroup\$

I am just getting started with C# and wanted to make sure that I am aware of any errors/inefficiencies in my code.

For this specific project, I wanted to prove to someone that a "bot" could easily understand phone numbers from Craigslist that are mixed (ex. 9eight7six54three2one0).

Here is what I have right now - it successfully outputs a string of numbers when fed a phone number such as the one above.

using System;

namespace Phone_Decoder
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Enter your mixed-format phone number:");

            string phone = Console.ReadLine();

            char[] b = new char[phone.Length];

            b = phone.ToCharArray();

            string finalphone = string.Empty;

            string number = string.Empty;

I am wondering if this "number" string should be a character array or not. It seems much easier to append a string than to add characters to an array, but I thought I read that strings are more memory-intensive.

            foreach (char c in b)
            {
                if (char.IsDigit(c))
                    finalphone += c;
                if (!char.IsDigit(c))
                {
                    number += c;
                    switch (number.ToLower())
                    {
                        case "zero":
                            finalphone += '0';
                            number = string.Empty;
                            break;
                        case "one":
                            finalphone += '1';
                            number = string.Empty;
                            break;
                        case "two":
                            finalphone += '2';
                            number = string.Empty;
                            break;
                        case "three":
                            finalphone += '3';
                            number = string.Empty;
                            break;
                        case "four":
                            finalphone += '4';
                            number = string.Empty;
                            break;
                        case "five":
                            finalphone += '5';
                            number = string.Empty;
                            break;
                        case "six":
                            finalphone += '6';
                            number = string.Empty;
                            break;
                        case "seven":
                            finalphone += '7';
                            number = string.Empty;
                            break;
                        case "eight":
                            finalphone += '8';
                            number = string.Empty;
                            break;
                        case "nine":
                            finalphone += '9';
                            number = string.Empty;
                            break;
                    }

I am a bit concerned with this switch statement, because there are many repeated lines ("number = string.Empty"). Could I make it more compact somehow?

                }
            }
            Console.WriteLine(finalphone);
            Console.ReadLine();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ FWIW, since every case sets number to string.Empty, you can simply do that outside of the switch block, before the foreach loop closes. Also, string concatenation can be very inefficient. If you know that you're going to be doing a bunch of concatenation, use a StringBuilder instead. However, @Denis' solution below is still the better approach. \$\endgroup\$ – Chris Pratt Jun 7 '17 at 12:49
7
\$\begingroup\$

You can extract that logic to a simple dedicated method.

Your solution is over-complicated for such a simple task so I will offer an alternative solution.

Let's inspect the problem -

You get some string as an input. This string might contain digits and it might also contain words that represent numbers. If there are such words we want to replace them with the respective digit.

We humans know that 'Zero' means 0 and 'One' means 1 because this is part of our vocabulary, if we are uncertain of some word we can look it up in the dictionary right? The human dictionaries are structured mostly in the following way - Word - Meaning.

Well luckily C# also has such data type. Conveniently enough it's called Dictionary! Using this data type we can construct much more readable and extendable code.

Let's start by declaring a Dictionary:

private static readonly Dictionary<string, int> numbers = new Dictionary<string, int>
{
    ["Zero"] = 0,
    ["One"] = 1,
    ["Two"] = 2,
    ["Three"] = 3,
    ["Four"] = 4,
    ["Five"] = 5,
    ["Six"] = 6,
    ["Seven"] = 7,
    ["Eight"] = 8,
    ["Nine"] = 9,
};

From here it's trivial to write a short method that does the job utilizing the dictionary numbers:

private static string ConvertFromMixedToStandardNumbers(string input)
{
    string result = input;
    foreach (var number in numbers)
    {
        result = result.Replace(number.Key.ToLower(), number.Value.ToString());
    }
    return result;
}

Or even shorter with LINQ:

private static string ConvertFromMixedToStandardNumbers(string input)
{
    return numbers.Aggregate(input,
        (current, number) => current.Replace(number.Key.ToLower(), number.Value.ToString()));
}

As you can see it's extremely easy to write short and humanly understandable code when you use the proper data structures and C# techniques.

You can even add an enum to the mix and create the dictionary from it:

private enum Number
{
    Zero,
    One,
    Two,
    Three,
    Four,
    Five,
    Six,
    Seven,
    Eight,
    Nine,
}

private static readonly Dictionary<string, int> numbers = MapEnum(typeof(Number));

private static Dictionary<string, int> MapEnum(Type enumType)
{
    return Enum.GetValues(enumType).Cast<object>()
        .ToDictionary(value => value.ToString(), value => (int) value);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Your dictionary example is very helpful. Just for others' reference, "System.Collections.Generic" must be imported to use dictionary. \$\endgroup\$ – user67339 Jun 5 '17 at 22:48
  • 1
    \$\begingroup\$ That's true, thanks for appreciating the idea. You can further research the Dictionary<TKey, TValue> type, it can come in handy in a lot of situations as it's really fast to access elements it also allows to easily map objects as illustrated in the answer. \$\endgroup\$ – Denis Jun 5 '17 at 22:53
  • \$\begingroup\$ Why would you create a new string on every ToLower() of the key, and a new string on every ToString() of the value, when you could just store lowercase keys and string values in the Dictionary in the first place? \$\endgroup\$ – 404 Jun 8 '17 at 10:52
  • \$\begingroup\$ That's a good point @eurotrash \$\endgroup\$ – Denis Jun 8 '17 at 14:27
4
\$\begingroup\$
char[] b = new char[phone.Length];

b = phone.ToCharArray();

You don't need two lines for it. You can assign the value in only one line:

char[] b = phone.ToCharArray();

Also the name b is not a good name. What does it stand for? You should always use meaningful names. For everything.

But even this is not necessary. You can enumerate a string so the loop could just be:

foreach(var c in phone)

number += c;

You should not concatenate strings in a loop with the + operator. Use a StringBuilder instead. Here it might not be that relevant but this is usually the most obvious bottleneck of string loops.


With a help of an enum you can make it a single Regex expression that replaces each word with a number:

enum Number 
{
    Zero,
    One,
    Two,
    Three,
    Four,
    Five,
    Six,
    Seven,
    Eight,
    Nine,
}

Example:

var number = "9eight7six54three2one0";
var result = 
    Regex
        .Replace(
            number,
            $"({string.Join("|", Enum.GetNames(typeof(Number)))})",
            m => ((int)Enum.Parse(typeof(Number), m.Groups[1].Value, true)).ToString(),
            RegexOptions.IgnoreCase); // 9876543210
\$\endgroup\$
  • \$\begingroup\$ Good info. I do have a few things that I'd like to clarify. Is the Regex/enum example a replacement for the switch statement, or the entire main loop? Also, what do you mean by a "bottleneck" for a string loop? \$\endgroup\$ – user67339 Jun 5 '17 at 20:56
  • 2
    \$\begingroup\$ @user67339 The whole loop: online example. String concatenation (finalPhone += c;) can be an expensive operation, and abusing it in a loop like this can hurt performance. "Bottleneck" refers to a point of code that acts like a choke point, usually for performance; string concatenation, like in your example, is a common culprit. Check out this StackOverflow answer. \$\endgroup\$ – ricksmt Jun 5 '17 at 23:44

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.