Converting numbers to English words

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
#include
#include

const std::vector 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 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 > answer) || ans

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

StackExchange Code Review Q#97166, answer score: 6