HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcsharpMinor

Convert number to words (web application)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numberapplicationconvertwordsweb

Problem

I'm looking for a new job, and a company who had a role I was going for asked me to do a programming exercise. It consisted of making a web application of two or more pages that took a person's name and a number, and then rendered the number converted to words. I sent it off and they replied saying that the code wasn't of the level they required so I'm hoping that you could help me and have a look at it so I know what I need to improve on.

It's a .Net 4 C# web application using Nunit for the unit tests, and I'm allowing the user to enter numbers up to a quadrillion (using the short scale), and a maximum of 13 decimals.

The zip of the full solution can be downloaded here.

aspx page:


    Exercise programming exercise
    
        Please enter your name and a number.
    
    
        Name:
        
        Number:
        
        
        
    

    Thank you
    
        Name entered:
        
        
        
        Number entered:
        
        
    

    
        Oops, something went wrong. Please try again later.
    


Code behind:

```
using System;
using Common;

namespace Exercise
{
public partial class _Default : System.Web.UI.Page
{
protected void Page_Load(object sender, EventArgs e)
{
}

protected void SubmitButton_Click(object sender, EventArgs e)
{
string numberAsWords = String.Empty;
decimal? decimalNumber = DecimalValidator.Validate(NumberTextBox.Text);

if (decimalNumber != null && decimalNumber < 1000000000000000000m)
{
decimal number = (decimal)decimalNumber;

try
{
numberAsWords = NumberToWordsConverter.Convert(number);

Name.Text = Server.HtmlEncode(NameTextBox.Text);
Number.Text = numberAsWords;
NameAndNumberForm.Visible = false;
SuccessMessage.Visible = true;
}
catch
{
// the relevant exceptions should be caug

Solution

I'm going to focus only on IntToWords(), nothing else (although I think using ASP.NET MVC is considered a better practice than plain ASP.NET).

I can see several problems with that code:

  • You repeat yourself too much. All the code from thousands up to quadrillions follows the same pattern.



  • You allocate the map arrays over and over. You should probably put them in a static field and initialize them only once.



  • You're doing quite a lot string concatenation, which creates quite a lot unnecessary garbage. You should use StringBuilder instead.



  • When you extract smaller parts of the number, you then send them to the full IntToWords(), which then unnecessarily checks for quadrillions, …, thousands. Extracting a method for the part that computes the small numbers would help you with that.



My solution would look like this:

private static readonly string[] UnitsMap = new[]
{
    "ZERO", "ONE", "TWO", "THREE", "FOUR", "FIVE", "SIX", "SEVEN", "EIGHT", "NINE", "TEN",
    "ELEVEN", "TWELVE", "THIRTEEN", "FOURTEEN", "FIFTEEN", "SIXTEEN", "SEVENTEEN",
    "EIGHTEEN", "NINETEEN"
};

private static readonly string[] TensMap = new[]
{
    "ZERO", "TEN", "TWENTY", "THIRTY", "FORTY",
    "FIFTY", "SIXTY", "SEVENTY", "EIGHTY", "NINETY"
};

private static readonly string[] ScaleMap = new[]
{ "", " THOUSAND", " MILLION", " BILLION", " TRILLION", " QUADRILLION" };

static IEnumerable SplitIntoThousands(long number)
{
    while (number != 0)
    {
        yield return (int)(number % 1000);
        number /= 1000;
    }
}

static string SmallNumberToWords(int number)
{
    string result = null;

    if (number > 0)
    {
        if (number >= 100)
        {
            var hundrets = SmallNumberToWords(number / 100);
            var tens = SmallNumberToWords(number % 100);

            result = hundrets + " HUNDRED";

            if (tens != null)
                result += ' ' + tens;
        }
        else if (number  0)
                result += "-" + UnitsMap[number % 10];
        }
    }

    return result;
}

public static string NumberToWords(long number)
{
    if (number == 0)
        return "ZERO";

    if (number = 0; i--)
    {
        var word = SmallNumberToWords(thousands[i]);

        if (word != null)
        {
            if (result.Length > 0)
            {
                if (i == 0)
                    result.Append(" AND ");
                else
                    result.Append(' ');
            }
            result.Append(word);
            result.Append(ScaleMap[i]);
        }
    }

    return result.ToString();
}


It still partially has problems 3 and 4, but in a much smaller amount and I think eliminating them completely would make the code too complicated (though in the case of problem 3, it could still be worth it, if this is performance-critical code).

Code Snippets

private static readonly string[] UnitsMap = new[]
{
    "ZERO", "ONE", "TWO", "THREE", "FOUR", "FIVE", "SIX", "SEVEN", "EIGHT", "NINE", "TEN",
    "ELEVEN", "TWELVE", "THIRTEEN", "FOURTEEN", "FIFTEEN", "SIXTEEN", "SEVENTEEN",
    "EIGHTEEN", "NINETEEN"
};

private static readonly string[] TensMap = new[]
{
    "ZERO", "TEN", "TWENTY", "THIRTY", "FORTY",
    "FIFTY", "SIXTY", "SEVENTY", "EIGHTY", "NINETY"
};

private static readonly string[] ScaleMap = new[]
{ "", " THOUSAND", " MILLION", " BILLION", " TRILLION", " QUADRILLION" };

static IEnumerable<int> SplitIntoThousands(long number)
{
    while (number != 0)
    {
        yield return (int)(number % 1000);
        number /= 1000;
    }
}

static string SmallNumberToWords(int number)
{
    string result = null;

    if (number > 0)
    {
        if (number >= 100)
        {
            var hundrets = SmallNumberToWords(number / 100);
            var tens = SmallNumberToWords(number % 100);

            result = hundrets + " HUNDRED";

            if (tens != null)
                result += ' ' + tens;
        }
        else if (number < 20)
            result = UnitsMap[number];
        else
        {
            result = TensMap[number / 10];
            if ((number % 10) > 0)
                result += "-" + UnitsMap[number % 10];
        }
    }

    return result;
}

public static string NumberToWords(long number)
{
    if (number == 0)
        return "ZERO";

    if (number < 0)
        return "MINUS " + IntToWords(-number);

    var thousands = SplitIntoThousands(number).ToArray();

    var result = new StringBuilder();

    for (int i = thousands.Length - 1; i >= 0; i--)
    {
        var word = SmallNumberToWords(thousands[i]);

        if (word != null)
        {
            if (result.Length > 0)
            {
                if (i == 0)
                    result.Append(" AND ");
                else
                    result.Append(' ');
            }
            result.Append(word);
            result.Append(ScaleMap[i]);
        }
    }

    return result.ToString();
}

Context

StackExchange Code Review Q#14033, answer score: 4

Revisions (0)

No revisions yet.