Number to English word converter

Question

This is a simple program that I wrote. It takes a 64-bit unsigned int as input and returns its English name.

#include <iostream>
#include <string>
#include <cmath>
#include <stdint-gcc.h>
using namespace std;
int container[7];                       /// every element contains a number between 0 and 999
string conv0to19(uint64_t);   /// converts 1 to 19 to its name
string conv_dec(uint64_t);    /// converts 20,30...,90 to its name
string convert(uint64_t);       ///converts a number from of range uint64_t to its name
void fillContainer(uint64_t);
string convert1to999(uint64_t, int);

string name[]={"","thousand","million","billion","trillion","quadrillion","quintillion"};

string conv0to19(uint64_t a)
{
    switch (a)
    {
        case 0:return "";   break;
        case 1:return "one ";  break;
        case 2:return "two ";  break;
        case 3:return "three ";break;
        case 4:return "four "; break;
        case 5:return "five "; break;
        case 6:return "six ";  break;
        case 7:return "seven ";break;
        case 8:return "eight ";break;
        case 9:return "nine "; break;
        case 10:return "ten ";  break;
        case 11:return "eleven "; break;
        case 12:return "twelve "; break;
        case 13:return "thirteen ";break;
        case 14:return "fourteen "; break;
        case 15:return "fifteen "; break;
        case 16:return "sixteen "; break;
        case 17:return "seventeen ";break;
        case 18:return "eighteen ";break;
        case 19:return "nineteen ";break;
    }
}

string conv_dec(uint64_t a)
{
    switch(a)
    {
        case 0:return "";break;
        case 1:return "";break;
        case 2:return "twenty ";   break;
        case 3:return "thirty ";  break;
        case 4:return "forty ";  break;
        case 5:return "fifty "; break;
        case 6:return "sixty "; break;
        case 7:return "seventy "; break;
        case 8:return "eighty ";  break;
        case 9:return "ninety ";break;

    }
}

string convert1to999(uint64_t a,int i)
{
    if(a!=0)
    {
        if (a>0&&a<20)
            return conv0to19(a)+name[i];

        if(a>20&&a<100&&a%10!=0)
            return conv_dec(a/10)+conv0to19(a%10)+name[i];

        if(a>=20&&a&&a<100&&a%10==0)
            return conv_dec(a/10)+name[i];

        if(a>=100&&a<1000)
            return conv0to19(a/100)+"hundred "+conv_dec(fmod((a/10),10))+conv0to19(a%10)+name[i];
    }
    else return "";
}


void fillContainer(uint64_t a)
{
    for(int i=0;i<=6;i++)
    {
        container[i]=a%1000;
        a/=1000;
    }
}

string convert(uint64_t a)
{
    if(a!=0)
    {
        string output;
        fillContainer(a);

        for(int i=6;i>=0;i--)
            if(container[i]!=0)
                output=output+convert1to999(container[i],i)+" ";

        while(output[output.size()-1]==' ')         ///deletes the
        output.resize(output.size()-1);            ///white spaces on the end

        return output;
    }
    else
        return "zero";
}

int main()
{
    uint64_t a;
    cin>>a;
    cout<<convert(a);
}

Answer 1

Switched

Let's start with the obvious things we can improve. A return statement terminates the current function. Therefore, all your

case XY: return AB; break;

can be rewritten as

case XY: return AB;

However, you don't have any default cases in both switch statements. What happens if you use conv0to19(100)? Well, it's not obvious, because it's undefined behaviour.

For the sake of simplicity, let's use an assert for unhandled cases. But before we do that, let's have a closer look at your switch:

    case 0:return "";
    case 1:return "one ";
    case 2:return "two ";
      ...
    case 18:return "eighteen ";
    case 19:return "nineteen ";

Hm. You have a continuous range of integer values. So let's use that to our advantage:

std::string conv0to19(uint64_t a) {
    static char const * const numbers[20] = 
      {"", "one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten",
      "eleven", "twelve", "thirteen", "fourteen", "fifteen", "sixteen", "seventeen", "eighteen", "nineteen"
      };

    assert(0 <= a && a < 20);

    return numbers[a];
}

You can do the same for conv_dec. Note that I've removed the suffix space from the string. More on that later.

While we're at it and using an array, your name should:

• have a better name (especially not name, since it's a collection of several ones)
• be const, so that you don't change it by accident:

const std::string numberNames[] = {"", "thousand", "million", "billion", "trillion", "quadrillion", "quintillion"};

By the way, did you notice that I've added whitespace to separate the arguments? Remember code is written for humans (and for machines). If you want to remove a word later, it should be easy to do so, and whitespace makes it a lot easier to mark text with Ctrl+Shift (or whatever your operating system/editor uses).

Back to the beginning

int container[7];                       /// every element contains a number between 0 and 999

That is a code smell. It's a global variable, which isn't constant. And the description is, well, no really helpful. Just remove it.

Also, the 7 seems like a magic number. Why is it 7? Why not 10?

Either way, you should include cstdint instead of stdint-gcc.h, or — if your compiler is too old — stdint.h. Also, I recommend you to typedef your used number type, e.g.:

typedef uint64_t number_type;

That way you can replace the type easily, if you want to support even larger numbers later.

And using namespace std… well, it's considered bad practice for large programs, especially in headers, but you're writing only a single file here. It's up to you, but I wouldn't use it.

Using logic to reduce if-complexity

Your convert1to999 is too complex. If we didn't enter the following block:

    if (a>0&&a<20)
        return conv0to19(a)+name[i];

we're guaranteed that a is larger then 20. Why? Because a isn't 0. Since it is a non-negative number, it must be then greater than zero, so a>0 holds, so a < 20 must not hold. We don't need to repeat that in the next condition:

    if(a>20&&a<100&&a%10!=0)
       ^^^^

And again, your code is neigh unreadable, since you don't use any whitespace. What is easier to read:

a>=20&&a&&a<100&&a%10==0

or

a >= 20 && a && a < 100 && a % 10 == 0

I guess you now see the superfluous a in the condition, which will always be true at that point.

Next, this function should do one job. It should get a number from 0-999 and return its name, not add the suffix. So let's get rid of that and fix the if complexity. And while we're at it, let's add another function for 0-99:

std::string conv0to99(number_type a)
{
    assert(0 <= a && a < 100);

    if(a == 0) {
        return "";
    } else if (a <= 20) {
        return conv0to19(a);
    } else {
        return conv_dec(a / 10) + "-" + conv0to19(a % 10);
    }
}

std::string convert1to999(number_type a)
{
    assert(0 <= a && a < 1000);

    if(a == 0) {
        return "";
    } else if (a <= 100) {
        return conv0to99(a);
    } else {
        return conv0to19(a/100) + " hundred " + conv0to99(a % 100);
    }
}

Both functions are rather simple and easy to check. The hurdles of the <=20 are now (almost) entirely hidden in conv0to99, which is still easy enough to understand quickly.

As I said, get rid of fillContainer. You use it only in convert, so let's only have it there. Also, handle the easy case first:

std::string convert(uint64_t a)
{
    if(a == 0) {
        return "zero";
    }

    std::string output;
    int container[7];

    for(int i = 0; i < 7; ++i) {
        container[i] = a % 1000;
        a = a / 1000;
    }

    for(int i = 6; i>=0; i--) {
        if(container[i] != 0) {
            // we add the name now here instead of in convert1to999
            output += convert1to999(container[i]) + " " + name[i] + " ";
        }
    }

    // if your compiler doesn't have `output.back()`, use your old check
    while(output.back() == ' ') {
        output.pop_back();
    }

    return output;
}

You can also get rid of container if you build the number from the back.

Last words

Make sure to format your code. Either use an editor which supports automatic formatting, or use a program that formats your code, especially if you want to share your code.

Answer 2

Overall your solution is straightforward and clear.

A few areas might be improved including

• consistency of formatting,
• enabling and addressing compiler warnings,
• considering brevity/efficiency/constancy of your algorithm.

Formatting

Whitespace

From experience, even though whitespace does not matter to the compiler, I recommend that you apply it as consistently as possible and to help the reader see the structure.

While subjective, my opinion is that your code would be cleaner if it had whitespace after the includes and before prototypes, aligned adjacent inline comments, consistently used single spaces or alignment after punctuation and around operators.

I consider the readability of

if(a>20&&a<100&&a%10!=0)
    return conv_dec(a/10)+conv0to19(a%10)+name[i];

to be improved as

if (a > 20 && a < 100 && a % 10 != 0)
    return conv_dec(a / 10) + conv0to19(a % 10) + name[i];

Single Statement Braces

My opinion is that the lack of braces around single-line if/for/while statement eventually causes far more pain than it saves. Take my experience for what you will.

Consistency

Whatever styling you choose, please apply it consistently. Longterm you might consider applying a code formatter to help maintain formatting consistency within your codebase.

Enable Compiler Warnings

Compile with warnings enabled and consider what they say.

My compiler informed me that several functions had code paths that failed to return the expected value. Specifically in this case, all case-statements should have a default case, whether it be a return or an exception. Plus in convert1to999 consider what happens if a == 1000 due to some code changes or function reuse.

Algorithm and Coding

Eliminate code whenever possible unless it trades off on clarity. The function convert1to999 takes an unsigned input so it will never be < 0 so do not test for it. In fact there is also have another extraneous check of a in a later condition obscured by the lack of spaces. conv0to19 handles 0 input correctly so you don't need special handling for (a % 10) == 0.

(Keeping your braces formatting style even though I prefer a terser style. Using else if because it clearly conveys intent without needing to immediately recognize multiple return paths.)

string convert1to999(uint64_t a, int i)
{
    if(a != 0)
    {
        boolean isNonMag = (a % 10 != 0);
        if (a < 20)
        {
            return conv0to19(a) + name[i];
        }
        else if (a < 100)
        {
            // note conv0to19 handle 0 properly
            return conv_dec(a / 10) + conv0to19(a % 10) + name[i];
        }
        else if(a < 1000)
        {
            return conv0to19(a / 100) + "hundred " + conv_dec(fmod((a / 10), 10)) + conv0to19(a % 10) + name[i];
        }
    }

    return "";
}

The use of case statements in conv1to19 and conv_dec is certainly reasonable but the problem maps so naturally to array indexing. Furthermore you do use array indexing for handling order of magnitude counters. There is a consistency argument for using it more widely.

Of course the downside of array indexing is that it opens the prospect of array indexing errors. On option is to use a std::vector instead. Especially if you do use arrays I strongly suggest that you get in the habit of bounds checking input and in cases where you have an expected array length that you encode the array length into the declaration to help avoid hard to catch mistakes.

Particularly since you do use an indexed array for the order of magnitude counters, you might consider using them elsewhere for the sake of constancy.

string lessThan20[20] = {
    "",
    "one ",
    "two ",
    "three ",
    "four ",
    "five ",
    "six ",
    "seven ",
    "eight ",
    "nine ",
    "ten ",
    "eleven ",
    "twelve ",
    "thirteen ",
    "fourteen ",
    "fifteen ",
    "sixteen ",
    "seventeen ",
    "eighteen ",
    "nineteen "
};

string conv1to19(uint64 a) {
    if (a < 20) {
        return lessThan20[a];
    }

    return "";
}

Certainly maintaining the use of case is perfectly defensible in my mind.

Generally, globals limits your ability to reuse the code.

int container[7];                       /// every element contains a number between 0 and 999

Consider passing an array to populate to fillContainer. (Also consider whether the naming could be improved, perhaps something like convertToMagnitudeEncoding).

#define ENCODED_LENGTH 7

...
{
    ...
    int magnitudeEncoded[ENCODED_LENGTH];
    fillContainer(a, &decimalEncode);
    ...
}

I suggest that except when the for loop boundary is clearer with <= for your array bounds, prefer < as an idiom.

void fillContainer(uint64_t a, int* decimalEncoded)
{
    for(int i = 0; i < ENCODED_LENGTH; i++)
    {
        decimalEncoded[i] = a % 1000;
        a /= 1000;
    }
}

Better might be to use or return a safe container or pass an array length along with the array rather than to have the length embed in physically separated parts of code.

Other observations

1. No bounds checking value used when indexing into name
2. Avoid scattering magic numbers 6 and 7 through the code. Use constants or defines if you will reuse them. This mitigates introducing errors due to partial changes at some later date.
3. When possible avoid mixing snake and camel function naming conventions such as conv_dec and fillContainer.
4. Usually one want to terminate console output with newlines << endl;