Converting numbers to English words

Question

This is a simple program to convert a number, from 0 up to 99999999, to English words.

Example:

If the input is:

1234

The output would be:

one thousand two hundred thirty four

I would like to get feedback regarding this small program.

#include <iostream>
#include <vector>
#include <string>

const std::vector<std::string> words[] = 
{
    {
        "", 
        "one", 
        "two", 
        "three", 
        "four", 
        "five", 
        "six", 
        "seven", 
        "eight", 
        "nine",
        "ten",
        "eleven",
        "twelve", 
        "thirteen", 
        "fourteen", 
        "fifteen", 
        "sixteen", 
        "seventeen", 
        "eighteen", 
        "nineteen" 
    },
    {
         "-", 
         "-", 
         "twenty -",
         "thirty -",
         "forty -", 
         "fifty -", 
         "sixty -", 
         "seventy -", 
         "eighty -", 
         "ninety -"
    },
    {
         "million",
         "hundred",
         "thousand",
         "hundred",
         ""
    },
};

class NumberConvertor
{
public:
    NumberConvertor(std::size_t number);

    friend std::ostream& operator<< (std::ostream& out, const NumberConvertor& nc);

private:    
    void convert(std::size_t unit, const std::string& units);

    std::size_t mNumber;
    std::string mResult;
    std::size_t mIterator;
    std::size_t mSwap;
};

NumberConvertor::NumberConvertor(std::size_t number)
    : mNumber(number), mIterator(0), mSwap(0)
{
    if (mNumber == 0) 
    {
        mResult = "zero";
    }
    else
    {
        convert( 1000000,  words[2][0]);
    }
}

void NumberConvertor::convert(std::size_t unit, const std::string& units)
{
    std::size_t index = mNumber / (unit * 10);
    std::size_t sub   = (index > 1 ? index * unit * 10 : 0); 

    std::string word = words[1][index];

    if (word[word.size() - 1] == '-')
    {
        word.erase(word.size() - 1);
        index   = (mNumber-sub) / unit;
        word.append(words[0][index]);
    }

    if (word != "") 
    {
        std::size_t x = mNumber / 1000;

        switch(x)
        {
            case 100:
            case 200:
            case 300:
            case 400:
            case 500:
            case 600:
            case 700:
            case 800:
            case 900:
            {
                mResult.append( word + ' ' + units + ' ' + words[2][mIterator + 1] + ' ');
                break;
            }
            default:
            {
                mResult.append( word + ' ' + units + ' ');
            }
        };
    }

    mNumber -= mNumber / unit * unit;

    if( ++mIterator < words[2].size())
    {
        mSwap ^= 1;

        if(unit == 1)
        {
            convert( 1, words[2][0]);
        }

        convert( unit / (mSwap == 0 ? 100 : 10), words[2][mIterator]);              
    }
}

std::ostream& operator<< (std::ostream& out, const NumberConvertor& nc)
{
    return out << nc.mResult;
}

int readInput(const std::string& prompt) 
{
    int answer;
    while((std::cout << prompt) && (!(std::cin >> answer) || answer < 0 || answer > 99999999))
    {
        std::cout << "Enter a positive number between 0 - 99999999\n";
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    }
    return answer;
}

int main()
{
    while(1) // (ctrl c) to exit
    {
        std::cout << NumberConvertor(readInput("Enter the number: ")) << '\n';
    }
}

Answer 1

Here are some things that may help you improve your program.

Make sure to #include all required headers

This program refers to std::numeric_limits but does not include the corresponding header. Fix that by adding this line:

#include <limits>

Think about the user of the code

The point to this code, as you have already summarized in the title, is to create an English language representation of a number. That suggests a function to me, rather than a class. A class might be useful to keep helper functions and associated temporary data together, but it should probably be a static class. I'll show an example later in this answer. The function that would probably be most useful would have a signature like this:

std::string numToEnglish(int n);

A good way to design software, regardless of programming language, is often to first think of how it will be used. Formally these are called "Use Cases" but it doesn't have to be that formal. In the case of the design of a function or class that someone else might use, think of what you'd want if you were using it.

Keep your strings together

The lonely word "zero" is not with the other strings. This may work but if you ever want, say, to create a French language version or a German language version, the translation might miss that if it's not in the same place as all of the other numeric names.

Use local variable rather than member variables

The only time the member variables are used are in the one public function. This strongly suggests that they should be local to that function rather than member variables.

Think carefully about the algorithm

In English, we can convert to a string value by grouping by thousands. That is, "345" would be "three hundred forty five" and "345000" would be "three hundred forty five thousand". We can structure the conversion to take advantage of this regularity by having two functions. One main function that does the conversion and a helper function that takes a number less than 1000 and converts just that chunk to words.

Encapsulate static data where practical

The words array is only used by the conversion and is probably too specialized to be of general use. For that reason its scope could be reduced. First you could declare it as static so that it would only have file scope, but a better approach might be to make it a static data member of a class.

Use better naming

The vector named words is not well named. A better name might be EnglishWords or maybe EnglishNumberWords.

Putting it all together

First a class to do these things might look like this:

class NumberConvert
{
public:
    static std::string toEnglish(std::size_t number);
private:
    NumberConvert() = delete;
    static std::string convertEnglish(std::size_t number);
    static const std::vector<std::string> EnglishWords[3];
};

As you can see, all members are static and only the toEnglish function is public. Also the constructor is private and deleted meaning that this class cannot be instantiated. That is appropriate for this class. Next is the instantiation of the EnglishWords array:

const std::vector<std::string> NumberConvert::EnglishWords[3] = 
{
    {
        "zero", 
        "one", 
        "two", 
        "three", 
        "four", 
        "five", 
        "six", 
        "seven", 
        "eight", 
        "nine",
        "ten",
        "eleven",
        "twelve", 
        "thirteen", 
        "fourteen", 
        "fifteen", 
        "sixteen", 
        "seventeen", 
        "eighteen", 
        "nineteen" 
    },
    {
         "", 
         "", 
         "twenty",
         "thirty",
         "forty", 
         "fifty", 
         "sixty", 
         "seventy", 
         "eighty", 
         "ninety",
         "hundred"
    },
    {
        "", 
        " thousand", 
        " million", 
        " billion", 
        " trillion"
    },
};

Note that I have added "zero", and "hundred" to the first and second groups, respectively, and revised the last group to be solely names for powers of 1000. First, let's see how the overall routine works:

std::string NumberConvert::toEnglish(std::size_t number)
{
    if (number == 0) 
    {
        return EnglishWords[0][number];
    }
    std::string words;
    for (int thousands = 0; number; ++thousands, number /= 1000)
    {
        int chunk = number % 1000;
        if (chunk) {
            words = NumberConvert::convertEnglish(chunk) 
                + NumberConvert::EnglishWords[2][thousands] 
                + (words.size() ? " " : "") 
                + words;
        }
    }
    return words;
}

A few moments study and I'm sure you'll see that this is simply grouping things by powers of 1000 and converting each chunk. It converts from the smallest to largest grouping of digits. Now all that remains is to write the part that converts each chunk:

std::string NumberConvert::convertEnglish(std::size_t number)
{
    if (number == 0)
        return "";
    int hundreds = number / 100;
    number %= 100;   // just keep last two digits
    std::string words;

    if (hundreds) {
        words = NumberConvert::EnglishWords[0][hundreds] 
            + " " + NumberConvert::EnglishWords[1][10] 
            + (number ? " " : "");
    }
    if (number == 0) {
        return words;
    }
    if (number < 20) {
        return words + NumberConvert::EnglishWords[0][number];
    }
    int tens = number / 10;
    int units = number % 10;
    if (tens) {
        words += NumberConvert::EnglishWords[1][tens];
    }
    if (units) {
        words += (tens ? " " : "") + NumberConvert::EnglishWords[0][units];
    }
    return words;
}

Now a short test program:

int main()
{
    std::vector<size_t> testnumbers{ 
        0, 1, 8, 13, 43, 99, 100, 1234, 123456, 1234567, 1000567 
    };
    for (auto n : testnumbers) {
        std::cout << n << " = " << NumberConvert::toEnglish(n) << std::endl;
    }
}

Sample output

0 = zero
1 = one
8 = eight
13 = thirteen
43 = forty three
99 = ninety nine
100 = one hundred
1234 = one thousand two hundred thirty four
123456 = one hundred twenty three thousand four hundred fifty six
1234567 = one million two hundred thirty four thousand five hundred sixty seven
1000567 = one million five hundred sixty seven

Answer 2

Short review: The purpose of code is to tell other programmers what you're telling the computer to do. Your code here reads like you're trying show off all these cool things you can do - which makes for very difficult to read, unimplementable code. Don't perform multiple operations and checks on one line. Don't use member variables to do temporary state. Structure your code to make your intent as readily apparent to the reader as possible.

readInput

Seriously, just no:

while((std::cout << prompt) && (!(std::cin >> answer) || answer < 0 || answer > 99999999))

There is never any reason to write code like that. Why even pass a prompt to the function if you're going to write a different string anyway? I don't see a reason for the prompt.

main

You should write a function that looks like:

std::string convertNumber(int );

Having a streamable class is gratuitous.

NumberConverter

The flow of this class doesn't make much logical sense. You have a member variable mNumber, that you're modifying as you go. It's really more like an implicit argument to convert(). Same with all the member variables actually. Makes it very difficult to reason about the correctness of convert().

words[] doesn't make sense as an array. The three elements in the array have nothing to do with each other. You have the numbers under 20, you have the tens numbers (why the hyphen?) and the block identifiers. Declaring it this way means you have a bunch of cryptic words[2][0] or words[1][index] expressions. Prefer to make three different vectors and name them appropriately.

So convert() takes both a units and its string representation? That's not good design since you're basically passing the same/similar information twice. Prefer to restructure this call such that you're converting one "block" at a time (e.g. the millions block, then the thousands block, then the unit block). Redundant information is bad.

This fragment doesn't make sense either:

std::string word = words[1][index];

if (word[word.size() - 1] == '-')

Why are you using the specific word format as a signal for something? Also, all the words in words[1] end in a hyphen so you always go into this block.