Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Please review my code

import java.lang.*;
import java.util.*;
import java.io.*;


class NumberToWords
{
  static public boolean HelperConvertNumberToText(int num, String[] result)
  {
      String [] strones = {
        "One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight",
        "Nine", "Ten", "Eleven", "Twelve", "Thirteen", "Fourteen",
        "Fifteen", "Sixteen", "Seventeen", "Eighteen", "Nineteen",
      };

      String [] strtens = {
          "Ten", "Twenty", "Thirty", "Fourty", "Fifty", "Sixty",
          "Seventy", "Eighty", "Ninety", "Hundred"
      };

      result[0] = "";
      int single, tens, hundreds;

      if(num > 1000)
          return false;

      hundreds = num / 100;
      num = num - hundreds * 100;
      if( num < 20)
      {
          tens = 0; // special case
          single = num;
      }
      else
      {
          tens = num / 10;
          num = num - tens * 10;
          single = num;
      }

      if(hundreds > 0)
      {
         result[0] += strones[hundreds-1];
         result[0] += " Hundred ";
      }
      if(tens > 0)
      {
         result[0] += strtens[tens - 1];
         result[0] += " ";
      }
      if(single > 0)
      {
         result[0] += strones[single - 1];
         result[0] += " ";
      }
      return true;
  }

  static public boolean ConvertNumberToText(int num, String[] result)
  {
      String tempString[] = new String[1];
      tempString[0] = "";
      int thousands;
      int temp;
      result[0] = "";
      if(num < 0 || num > 100000)
      {
          System.out.println(num + " \tNot Supported");
          return false;
      }

      if( num == 0)
      {
         System.out.println(num + " \tZero");
         return false;
      }

      if(num < 1000)
      {  
          HelperConvertNumberToText(num, tempString);
          result[0] += tempString[0];
      }
      else
      {
          thousands = num / 1000;
          temp = num - thousands * 1000;
          HelperConvertNumberToText(thousands, tempString);
          result[0] += tempString[0];
          result[0] += "Thousand ";             
          HelperConvertNumberToText(temp, tempString);
          result[0] += tempString[0];
      }
      return true;
  }

  public static void main(String[] args)
  {
      String [] result = new String[1];
      result[0] = "";
      int  i, num;
      int [] arrNum =
      {
        -1, 0, 5, 10, 15, 19, 20, 21, 25, 33, 49, 50, 72,
        99, 100, 101, 117, 199, 200, 214, 517, 589, 999,
        1000, 1010, 1018, 1200, 9890, 10119, 13535, 57019,
        99999, 100000, 100001
      };

      for (i = 0; i < arrNum.length; i++)
      {
          num = arrNum[i];
          if( ConvertNumberToText(num, result) == true)
             System.out.println(num  + "\t" + result[0]);
      }
  }
}
share|improve this question

2 Answers 2

Firstly, you have a spelling error. "Fourty" should be "Forty".

Next, You have snippets like this:

num = num - hundreds * 100;

num = num - tens * 10;

temp = num - thousands * 1000;

These can be simplified using the modulo operator. The modulo operator (which is % in java) produces the remainder of a division. For example 1024 % 100 = 24 since 1024 divides into 100 ten times with a remainder of 24.

The snippets are then:

num %= 100;

num %= 10;

temp = num % 1000;

Additionally, you have unnecessary code which can be trimmed down because the results are unused. For instance, you don't use anything from any of the imports... so don't import them.

Lastly, some general code comments:

Why are your convertToString methods... returning booleans? If the arguments are invalid, then raise an Exception, don't return booleans. This would also simplify your code, as it would allow you to simply append the results together. This would also eliminate the need for passing a String[] around.

Also the Helper function should really be called ConvertLessThanThousand indicating what it does.

This is what I came up with:

class NumberToWords
{
  // No need to have these in the function. Unless you really care about hiding them... But reflection can still find these
  private static final String[] ONES = {
     "One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight",
     "Nine", "Ten", "Eleven", "Twelve", "Thirteen", "Fourteen",
     "Fifteen", "Sixteen", "Seventeen", "Eighteen", "Nineteen",
  };

  private static final String[] TENS = {
      "Ten", "Twenty", "Thirty", "Forty", "Fifty", "Sixty",
      "Seventy", "Eighty", "Ninety", "Hundred"
  };

  public String convertLessThanThousand(int num) {

      String result = "";
      int single, tens, hundreds;

      if(num > 1000)
          return result;

      hundreds = num / 100;
      num %= 100;
      if( num < 20) {
          tens = 0;
      }
      else {
          tens = num / 10;
          num %= 10;
      }
      single = num; // Notice I moved this outside since it is duplicated in both cases

      if(hundreds > 0) {
         result += ones[hundreds-1] + " Hundred ";
      }
      if(tens > 0) {
         result += tens[tens - 1] + " ";
      }
      if(single > 0) {
         result += ones[single - 1] + " ";
      }
      return result;
  }

  public static String convertNumberToText(int num)
  {
      String tempString = "";
      int thousands;
      int temp;
      String result = "";
      if(num < 0 || num > 100000) {
          throw new IllegalArgumentException("" + num + " \tNot Supported");
      }

      if( num == 0) {
         return "" + num + " \tZero";
      }

      if(num < 1000) {  
          result += ConvertLessThanThousand(num);
      }
      else {
          thousands = num / 1000;
          temp = num % 1000;
          tempString = ConvertLessThanThousand(thousands);
          result += tempString + "Thousand ";            
          result += ConvertLessThanThousand(temp);
      }
      return result
  }

  public static void main(String[] args)
  {
      int  i, num;
      int[] arrNum =
      {
        -1, 0, 5, 10, 15, 19, 20, 21, 25, 33, 49, 50, 72,
        99, 100, 101, 117, 199, 200, 214, 517, 589, 999,
        1000, 1010, 1018, 1200, 9890, 10119, 13535, 57019,
        99999, 100000, 100001
      };

      for (i = 0; i < arrNum.length; i++)
      {
          num = arrNum[i];

          try {
             System.out.println(num + "\t" + ConvertNumberToText(num));
          } catch (IllegalArgumentException e) {
              System.out.println(e.message());
          }
      }
  }
}

Lastly, if you are planning on expanding this to bigger and bigger numbers, I would suggest using a recursive approach, not unlike what I have implemented in Python in my question on this same site.

share|improve this answer
2  
An array String[] result is being used to emulate out-arguments, i.e. the array behaves similar to a pointer. If we just use a String result and reassign that identifier inside the function, then the value that has been passed in will not have changed (Java's functions are not call-by-reference). Your “fixed” code will not print out any words generated by HelperConvertNumberToText. As you point out in the next paragraph, the correct solution would be to return a string, and to throw exceptions on errors rather than returning status codes. –  amon Aug 23 at 9:46
1  
Ah, I forgot about Java's passing by value. Thanks, will move my suggesting to replace String[] down to where I suggest returning strings –  mleyfman Aug 23 at 15:05
1  
@amon fixed! Had to change my answer a bit –  mleyfman Aug 23 at 15:25

"Strone"? What's a "strone"? Oh… of course!

In seriousness, naming could be improved. They are obviously arrays of strings; the Hungarian prefix is unnecessary. Also, as constants, they should be declared static final and named with ALL_CAPS.

public class NumberToWords {
    private static final ONES = { "One", "Two", … };
    private static final TENS = { "Ten", "Twenty", … };
    …
}

Helper functions should be private.

It is customary to put the static keyword after the access modifier, as in public static void main(…). It is unconventional to say static public boolean ConvertNumberToText(…). Also, functions should be namedStartingWithLowerCase().

Using a single-element array as a parameter so that you can pass back the result as an out-parameter is very weird. Using a StringBuilder would be a bit more obvious, but still very odd. The right approach should be to return a String, and throw an exception if necessary — perhaps NumberFormatException. Ideally, your code should be able to handle every possible int input, making errors impossible.

share|improve this answer
1  
+1 for making me laugh. Kill the Hungarian Notation with fire! –  RubberDuck Aug 23 at 12:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.