Turning numbers into text

Question

Here is my program that turns numbers into text. It can be extended to millions, billions, trillions and so on. Please review my code and here is what I expect from the users:

1. Is there any better variable names, function names, you can give me? Or my given names are okay? If you've better variable name ideas, please share.

2. Last time one expert user gave me suggestion to write functions in separate files and then implement into main function by calling header name, I just did that, is this okay? Or I should make more separate files for each custom function?

3. I tried my best to write program in object oriented way. I used Class and then wrote some functions inside them so I can call any method after the object name.

   For example: number.validate(varName)

   Do you see any problems in that? Am I doing something that isn't a good programming practice?

4. My program works perfectly without any problem, is there any better way to do this?

1. main.cpp

/*
 * Implement the source code that turns numbers into English text.
 */
#include <iostream>
#include "numberToText.hpp"
int main (void)
{
 numberToText();
 return 0;
}

2. numberToText.cpp

#include <iostream>
#include <string>
#include "textlib.hpp"
class num
{
 public:
 bool validate (std::string numStr)
 {
 for (int i = 0; i < numStr.length(); i++)
 {
 if (!(isdigit(numStr[i])))
 {
 return false; // terminate loop by returning value if there's anything in this string apart from number
 }
 }
 return true; // if there is no problem in loop then return true to confirm that string has all the numbers.
 }
 void take (std::string &numberString)
 {
 std:: cout << "Enter your number:\n";
 std::cin >> numberString;
 while (1)
 {
 if (validate(numberString))
 {
 break; //SUCCESS
 }
 else
 {
 std::cout << "Invalid number, try again:\n";
 std::cin >> numberString;
 }
 }
 }
 void text (std::string string)
 {
 int digitLength = string.length();
 if (digitLength == 1)
 {
 one_Nine(0, string);
 }
 else if (digitLength == 2)
 {
 twoDigits(0, string);
 }
 else if (digitLength == 3)
 {
 threeDigits(0, string);
 }
 else if (digitLength > 3 && digitLength < 7)
 {
 thousand(0, string, digitLength);
 }
 else // add more conditions if you want to extend the program
 {
 std::cout << "Sorry, that's a vary large number.\n";
 }
 }
 private:
};
void numberToText (void)
{
 std::string numStr;
 num number;
 number.take(numStr);
 number.validate(numStr);
 number.text(numStr);
}

3. numberToText.hpp

/*
 * take numbers and display text
 */
#ifndef NUMBERTOTEXT_HPP_
#define NUMBERTOTEXT_HPP_
void numberToText (void);
#endif

4. textlib.cpp

#include <iostream>
#include <string>
#include "textlib.hpp"
void one_Nine (int i, std::string numberString)
{
 if (numberString[i] == '1')
 {
 std::cout << "One ";
 }
 else if (numberString[i] == '2')
 {
 std::cout << "Two ";
 }
 else if (numberString[i] == '3')
 {
 std::cout << "Three ";
 }
 else if (numberString[i] == '4')
 {
 std::cout << "Four ";
 }
 else if (numberString[i] == '5')
 {
 std::cout << "Five ";
 }
 else if (numberString[i] == '6')
 {
 std::cout << "Six ";
 }
 else if (numberString[i] == '7')
 {
 std::cout << "Seven ";
 }
 else if (numberString[i] == '8')
 {
 std::cout << "Eight ";
 }
 else if (numberString[i] == '9')
 {
 std::cout << "Nine ";
 }
 else if (numberString[0] == '0')
 {
 std::cout << "Zero\n";
 }
 else
 {
 std::cout << "Error: one_Nine() function isn't working!\n";
 }
}
void twoDigits (int j, std::string numberString)
{
 if (numberString[j] == '1')
 {
 if (numberString[j+1] == '0')
 {
 std::cout << "Ten ";
 }
 else if (numberString[j+1] == '1')
 {
 std::cout << "Eleven ";
 }
 else if (numberString[j+1] == '2')
 {
 std::cout << "Twelve ";
 }
 else if (numberString[j+1] == '3')
 {
 std::cout << "Thirteen ";
 }
 else if (numberString[j+1] == '4')
 {
 std::cout << "Fourteen ";
 }
 else if (numberString[j+1] == '5')
 {
 std::cout << "Fifteen ";
 }
 else if (numberString[j+1] == '6')
 {
 std::cout << "Sixteen ";
 }
 else if (numberString[j+1] == '7')
 {
 std::cout << "Seventeen ";
 }
 else if (numberString[j+1] == '8')
 {
 std::cout << "Eighteen ";
 }
 else if (numberString[j+1] == '9')
 {
 std::cout << "Nineteen ";
 }
 else
 {
 std::cout << "Error: twoDigits().\n";
 }
 }
 else if (numberString[j] == '2')
 {
 std::cout << "Twenty ";
 if (numberString[j+1] == '0')
 {
 std::cout << "";
 }
 else
 {
 one_Nine(j+1, numberString);
 }
 }
 else if (numberString[j] == '3')
 {
 std::cout << "Thirty ";
 if (numberString[j+1] == '0')
 {
 std::cout << "";
 }
 else
 {
 one_Nine(j+1, numberString);
 }
 }
 else if (numberString[j] == '4')
 {
 std::cout << "Forty ";
 if (numberString[j+1] == '0')
 {
 std::cout << "";
 }
 else
 {
 one_Nine(j+1, numberString);
 }
 }
 else if (numberString[j] == '5')
 {
 std::cout << "Fifty ";
 if (numberString[j+1] == '0')
 {
 std::cout << "";
 }
 else
 {
 one_Nine(j+1, numberString);
 }
 }
 else if (numberString[j] == '6')
 {
 std::cout << "Sixty ";
 if (numberString[j+1] == '0')
 {
 std::cout << "";
 }
 else
 {
 one_Nine(j+1, numberString);
 }
 }
 else if (numberString[j] == '7')
 {
 std::cout << "Seventy ";
 if (numberString[j+1] == '0')
 {
 std::cout << "";
 }
 else
 {
 one_Nine(j+1, numberString);
 }
 }
 else if (numberString[j] == '8')
 {
 std::cout << "Eighty ";
 if (numberString[j+1] == '0')
 {
 std::cout << "";
 }
 else
 {
 one_Nine(j+1, numberString);
 }
 }
 else if (numberString[j] == '9')
 {
 std::cout << "Ninety ";
 if (numberString[j+1] == '0')
 {
 std::cout << "";
 }
 else
 {
 one_Nine(j+1, numberString);
 }
 }
 else if (numberString[j] == '0')
 {
 one_Nine(j+1, numberString);
 }
 else
 {
 std::cout << "Error: **********^^*****************";
 }
}
void threeDigits (int k, std::string numberString)
{
 if (numberString[k] != '0')
 {
 one_Nine(k, numberString);
 std::cout << "Hundred ";
 }
 if (numberString[k+1] != '0' || numberString[k+2] != '0')
 {
 twoDigits(k+1, numberString);
 }
}
void thousand (int l, std::string numberString, int digitLength)
{
 if (digitLength == 4)
 {
 if (numberString[l] != '0')
 {
 one_Nine(l, numberString);
 std::cout << "Thousand ";
 }
 threeDigits(l+1, numberString);
 }
 else if (digitLength == 5)
 {
 if (numberString[l] != '0')
 {
 twoDigits(l, numberString);
 std::cout << "Thousand ";
 }
 else if (numberString[l] == '0' && numberString[l+1] != '0')
 {
 one_Nine(l, numberString);
 std::cout << "Thousand ";
 }
 else
 {
 std::cout << "";
 }
 threeDigits(l+2, numberString);
 }
 else if (digitLength == 6)
 {
 if (numberString[l] != '0')
 {
 threeDigits(l, numberString);
 std::cout << "Thousand ";
 }
 else if (numberString[l] == '0' && numberString[l+1] != '0')
 {
 twoDigits(l+1, numberString);
 std::cout << "Thousand ";
 }
 else if (numberString[l+2] != '0')
 {
 one_Nine(l+2, numberString);
 std::cout << "Thousand ";
 }
 else
 {
 std::cout << "";
 }
 threeDigits(l+3, numberString);
 }
 else
 {
 std::cout << "Error: ~~~~~~";
 }
}

5. textlib.hpp

/*
 * Can print numbers in English text.
 */
#ifndef TEXTLIB_HPP_
#define TEXTLIB_HPP_
void one_Nine (int i, std::string numberString);
void twoDigits (int j, std::string numberString);
void threeDigits (int k, std::string numberString);
void thousand (int l, std::string numberString, int digitLength);
#endif

Answer 1

C++ vs C style

In C++ you don't need to declare empty parameter lists as void. Replace int

int main (void)

by

int main()

You also don't need to return 0; at the end of main in C++. This is done automatically by the compiler if the end of the main function is reached.

Class design

Using a class in this context was not the best choice. When you look at your class you will notice that it has no member variables and only member functions.

This means that each function could have been declared static (which means it does not need an instance of this class to be called on). If you were to make this transformation (making all of these class member functions static) you would have another code smell: A class that contains no (per instance) data and only static functions should in fact be a namespace.

Applying this transformation would only change your code in numberToText to:

 std::string numStr;
 num::take(numStr);
 num::validate(numStr);
 num::text(numStr);

Function (signature) design

You clearly need to use the return values of functions! Let's have a look at num::take for example. It returns the value via a reference parameter and has the return type void. This is somewhat counterintuitive. When looking at a function and asking oneself what it returns the first look is for the return type which says void in your case. How about this function interface:

std::string take();

It clearly communicates that take returns a string. Looking at the previous calls take(string) looks like it is taking the string and doing something with it (rather than returning into it).

The name take seems a bit generic to me. What is taken? What the function actually does is querying the user for a valid number(string) until one is given. The name should reflect that. How about getNumberStringFromUser or something similar?

Don't break your loop unnecessarily

Looking at the code I wonder why you are using a break here:

while (1)
{
 if (validate(numberString))
 {
 break; //SUCCESS
 }
//...

Use the loop condition instead of breaking from the loop if it is more clear:

while (!validate(numberString))
{

Exiting the loop within its body forces the reader to understand the additional (unexpected) "branch" introduced at the break.

redundant validation

The string has already been validated in take so why validating it again and then discarding the return value of validate?

function names

The names of some functions are not well chosen:

• one_Nine -> printOnesDigitWord
• twoDigits -> printTensDigitWord
• threeDigits -> printHundredsDigitWord
• thousand -> printNumberAsWords

Algorithm

Your code looks very brittle and not that extensible. The interfaces of the printing functions are a bit complicated. Instead of passing the full number string and the index where to look you should pass only the part of the string that is relevant to the function. Another possibility would be to pass the value as an int. This would allow you to fit the function more easily (and portable) into arrays for example one_Nine:

std::string oneDigitToString(int digit)
{
 assert(0 <= digit && digit < 10);
 static const char *digitNames[] = {"Zero" "One", "Two", "Three", "Four", 
 "Five", "Six", "Seven", "Eight", "Nine" };
 return digitNames[digit];
}

Note that there are several improvements:

• reduced sideeffects by not calling std::cout (but now the caller has to print!)
• increased efficiency (you always get the result in one step instead of hopping through the cascading ifs)
• tighter interface (there are "less" wrong values that you can pass into this function)
• stricter error reporting: the assert enforces correct values being passed to the function. Breaking the assert condition gives a (runtime) error and tells the developer that it is the callers fault (assert guards preconditions!)

Side effects

In the previous example I have returned the digit word string instead of printing it to std::cout this reduces side effects and increases the reusability of the function. Lets say you want to have a GUI version of this program where the string is not presented on cout but in the window title. Using functions that return the result as a string makes this easy while using cout makes it hard (you would have to duplicate or rewrite the code).

Answer 2

Use return values instead of this output via arguments style.

Don't signal errors by printing something to std::cout, use exceptions and throw instead: http://en.cppreference.com/w/cpp/header/stdexcept You still can let the user know about the error by catching the exception and printing the reason (.what()).

A concept doesn't become object oriented by just copying the functions into a class. It also doens't make it less object oriented when there are free functions. Don't be afraid to just implement them as free functions if there is no reason why they should be together in a common class (as in your case).

To write void into empty parameter lists is pretty much C style and very uncommon in C++.

You work with signed ints but you don't really support negative values. You should think about changing at least one of them.

• Amession: Thanks for the info about the errors :) And I'll avoid writing void in empty parameter, I hope that's a good programming habit.

Answer 3

1. I would prefer to define each function by returning its execution status.

   #define SUCCESS 0
   #define FAILURE -1
   int functionName(.....)
   

   instead of,

   void functionName(.....)
   

2. Instead of taking "std::string string" as input to each function call, take numStr as a data member.

   private:
    std::string numStr;
   

   Implement the constructor as,

   num(std::string str) : numStr(str) {}
   

   Define the object and start calling the member functions as,

   std:: cout << "Enter your number:\n";
   std::cin >> numberString;
   num *pNumberClassInst = new num(numberString);
   if(pNumberClassInst->validate() == false)
   {
    return FAILURE;
   }
   

   And change the other member calls accordingly.

• Nobody moving away from SE: Using macros as (int) constants is very C-ish, an enum is much better suited for this, also having error return types on functions looks like C again and blocks the return slot for actual return values (errors are better communicated via exceptions which also makes it harder to ignore them accidentally). The code in your constructor example does not look valid, what is the meaning of numstr(std::string numStr);?
• Subhajit: @Nobody, I edited my signature for the initializer list for the constructor, So what do you suggest,how the functions should be best executed so that the caller gets to know the status?
• Nobody moving away from SE: The usual error reporting mechanism in C++ is throwing of exceptions.