snippetcsharpMinor
Convert number to words (web application)
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:
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
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
I can see several problems with that code:
My solution would look like this:
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).
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
StringBuilderinstead.
- 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.