Convert number to words (web application)

Question

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:

<%@ Page Title="Programming exercise" Language="C#" MasterPageFile="/Site.master" AutoEventWireup="true" CodeBehind="Default.aspx.cs" Inherits="Exercise._Default" ViewStateMode="Disabled" ValidateRequest="false" %>
<asp:Content ID="BodyContent" runat="server" ContentPlaceHolderID="MainContent">
<asp:PlaceHolder ID="NameAndNumberForm" runat="server">
 <h1>Exercise programming exercise</h1>
 <p>
 Please enter your name and a number.
 </p>
 <fieldset>
 <label for="NameTextBox">Name:</label>
 <asp:TextBox ID="NameTextBox" runat="server"></asp:TextBox>
 <label for="NumberTextBox">Number:</label>
 <asp:TextBox ID="NumberTextBox" runat="server"></asp:TextBox>
 <asp:Label ID="ValidationError" runat="server" Visible="false" CssClass="error"></asp:Label>
 <asp:Button ID="SubmitButton" runat="server" Text="Submit" OnClick="SubmitButton_Click" />
 </fieldset>
</asp:PlaceHolder>
<asp:PlaceHolder ID="SuccessMessage" runat="server" Visible="false">
 <h1>Thank you</h1>
 <p>
 Name entered:
 <br />
 <asp:Literal ID="Name" runat="server" />
 <br /><br />
 Number entered:
 <br />
 <asp:Literal ID="Number" runat="server" />
 </p>
</asp:PlaceHolder>
<asp:PlaceHolder ID="ErrorMessage" runat="server" Visible="false">
 <p class="error">
 Oops, something went wrong. Please try again later.
 </p>
</asp:PlaceHolder>
</asp:Content>

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 caught and logged, but didn't do it for this exercise
 NameAndNumberForm.Visible = false;
 ErrorMessage.Visible = true;
 }
 }
 else
 {
 ValidationError.Visible = true;
 ValidationError.Text = "Please enter a valid number";
 }
 }
}
}

Decimal validator:

using System;
using System.Globalization;
namespace Common
{
public static class DecimalValidator
{
 public static decimal? Validate(string input)
 {
 // trim any whitespace from the number
 input = input.Replace(" ", String.Empty);
 decimal number;
 if (Decimal.TryParse(input,
 NumberStyles.Number,
 CultureInfo.CurrentCulture,
 out number))
 {
 return number;
 }
 return null;
 }
}
}

Number to words converter:

using System;
namespace Common
{
public static class NumberToWordsConverter
{
 public static string Convert(decimal number)
 {
 if (number == 0)
 return "ZERO";
 if (number < 0)
 return "MINUS " + Convert(Math.Abs(number));
 string words = String.Empty;
 long intPortion = (long)number;
 decimal fraction = (number - intPortion);
 int decimalPrecision = GetDecimalPrecision(number);
 fraction = CalculateFraction(decimalPrecision, fraction);
 long decPortion = (long)fraction;
 words = IntToWords(intPortion);
 if (decPortion > 0)
 {
 words += " POINT ";
 words += IntToWords(decPortion);
 }
 return words.Trim();
 }
 public static string IntToWords(long number)
 {
 if (number == 0)
 return "ZERO";
 if (number < 0)
 return "MINUS " + IntToWords(Math.Abs(number));
 string words = "";
 if ((number / 1000000000000000) > 0)
 {
 words += IntToWords(number / 1000000000000000) + " QUADRILLION ";
 number %= 1000000000000000;
 }
 if ((number / 1000000000000) > 0)
 {
 words += IntToWords(number / 1000000000000) + " TRILLION ";
 number %= 1000000000000;
 }
 if ((number / 1000000000) > 0)
 {
 words += IntToWords(number / 1000000000) + " BILLION ";
 number %= 1000000000;
 }
 if ((number / 1000000) > 0)
 {
 words += IntToWords(number / 1000000) + " MILLION ";
 number %= 1000000;
 }
 if ((number / 1000) > 0)
 {
 words += IntToWords(number / 1000) + " THOUSAND ";
 number %= 1000;
 }
 if ((number / 100) > 0)
 {
 words += IntToWords(number / 100) + " HUNDRED ";
 number %= 100;
 }
 if (number > 0)
 {
 if (words != String.Empty)
 words += "AND ";
 var unitsMap = new[] { "ZERO", "ONE", "TWO", "THREE", "FOUR", "FIVE", "SIX", "SEVEN", "EIGHT", "NINE", "TEN", "ELEVEN", "TWELVE", "THIRTEEN", "FOURTEEN", "FIFTEEN", "SIXTEEN", "SEVENTEEN", "EIGHTEEN", "NINETEEN" };
 var tensMap = new[] { "ZERO", "TEN", "TWENTY", "THIRTY", "FORTY", "FIFTY", "SIXTY", "SEVENTY", "EIGHTY", "NINETY" };
 if (number < 20)
 words += unitsMap[number];
 else
 {
 words += tensMap[number / 10];
 if ((number % 10) > 0)
 words += "-" + unitsMap[number % 10];
 }
 }
 return words.Trim();
 }
 private static int GetDecimalPrecision(decimal number)
 {
 return (Decimal.GetBits(number)[3] >> 16) & 0x000000FF;
 }
 private static decimal CalculateFraction(int decimalPrecision, decimal fraction)
 {
 switch(decimalPrecision)
 {
 case 1:
 return fraction * 10;
 case 2:
 return fraction * 100;
 case 3:
 return fraction * 1000;
 case 4:
 return fraction * 10000;
 case 5:
 return fraction * 100000;
 case 6:
 return fraction * 1000000;
 case 7:
 return fraction * 10000000;
 case 8:
 return fraction * 100000000;
 case 9:
 return fraction * 1000000000;
 case 10:
 return fraction * 10000000000;
 case 11:
 return fraction * 100000000000;
 case 12:
 return fraction * 1000000000000;
 case 13:
 return fraction * 10000000000000;
 default:
 return fraction * 10000000000000;
 }
 }
}
}

These are the unit tests, I'm only including the method names for brevity:

public void Add_DecimalNumber_ReturnDecimal(string number, decimal expected)
public void Add_Integer_ReturnDecimal(string number, decimal expected)
public void Add_NumberWithThousandSeparators_ReturnDecimal(string number, decimal expected)
public void Add_NumberWithSpaces_ReturnDecimal(string number, decimal expected)
public void Add_NegativeNumber_ReturnDecimal(string number, decimal expected)
public void Add_String_ReturnNull(string number, decimal? expected)
public void Add_SingleNumber_ReturnString(decimal number, string expected)
public void Add_TeenNumber_ReturnString(decimal number, string expected)
public void Add_TensNumber_ReturnString(decimal number, string expected)
public void Add_HundredsNumber_ReturnString(decimal number, string expected)
public void Add_ThousandsNumber_ReturnString(decimal number, string expected)
public void Add_MillionssNumber_ReturnString(decimal number, string expected)
public void Add_BillionssNumber_ReturnString(decimal number, string expected)
public void Add_TrillionssNumber_ReturnString(decimal number, string expected)
public void Add_QuadrillionssNumber_ReturnString(decimal number, string expected)
public void Add_OneDecimalNumber_ReturnString(decimal number, string expected)
public void Add_TwoDecimalNumber_ReturnString(decimal number, string expected)
public void Add_ThreeDecimalNumber_ReturnString(decimal number, string expected)
public void Add_FourDecimalNumber_ReturnString(decimal number, string expected)
public void Add_FiveDecimalNumber_ReturnString(decimal number, string expected)
public void Add_SixDecimalNumber_ReturnString(decimal number, string expected)
public void Add_SevenDecimalNumber_ReturnString(decimal number, string expected)
public void Add_EightDecimalNumber_ReturnString(decimal number, string expected)
public void Add_NineDecimalNumber_ReturnString(decimal number, string expected)
public void Add_TenDecimalNumber_ReturnString(decimal number, string expected)
public void Add_ElevenDecimalNumber_ReturnString(decimal number, string expected)
public void Add_TwelveDecimalNumber_ReturnString(decimal number, string expected)
public void Add_ThirteenDecimalNumber_ReturnString(decimal number, string expected)
public void Add_NegativeNumber_ReturnString(decimal number, string expected)

• hopingtoimprove: I've noticed that I forgot to remove the top 4 lines from IntToWords, I'll leave it in though so you can judge the code that I sent off.

Answer 1

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:

1. You repeat yourself too much. All the code from thousands up to quadrillions follows the same pattern.
2. You allocate the map arrays over and over. You should probably put them in a static field and initialize them only once.
3. You're doing quite a lot string concatenation, which creates quite a lot unnecessary garbage. You should use StringBuilder instead.
4. 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<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();
}

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).

• hopingtoimprove: Also, forgot to say, it said to use the language you had most experience with so I did it with web forms as I haven't got as much experience of MVC yet. For any new projects I'd definitely use MVC with Razor though.