3
\$\begingroup\$

I wrote some code to translate numbers ( for now just positive, up to the 32bit limit ) into English words.

Everything works and I'm happy. I searched through the site looking for a comparison code but I couldn't find a C++ version that goes above 999 to use as example

I had a few assumptions:

  • No need for and, just spaces ( Ex: two hundred three )

I never wrote much code ( yes this is a big program for me ), so I have no idea how to manage it correctly, I think it's pretty much spaghetti-code at the moment, that's why I tried adding some documentation to clarify things.

The algorithm I used feels like it's overcomplicated and more patched together than planned out.

#include <iostream>     // std::cout, std::cin, std::endl
#include <vector>       // std::vector
#include <algorithm>    // std::reverse

const std::vector<std::string> first_twenty_vocabular = {
        "zero ",
        "one ",
        "two ",
        "three ",
        "four ",
        "five ",
        "six ",
        "seven ",
        "eight ",
        "nine ",
        "ten ",
        "eleven ",
        "twelve ",
        "thirteen ",
        "fourteen ",
        "fifteen ",
        "sixteen ",
        "seventeen ",
        "eighteen ",
        "nineteen " };

const std::vector<std::string> magnitude_vocabular = {
        "hundred ",
        "thousand ",
        "million ",
        "billion "};

const std::vector<std::string> decine_vocabular = {
        "twenty ",
        "thirty ",
        "fourty ",
        "fifty ",
        "sixty ",
        "seventy ",
        "eighty ",
        "ninety "};

// HERE THE MAGIC HAPPENS.
std::string Stringer(std::vector<int> src);
        // Translate the number up to the hundreds, if the number is bigger it sends it to MagnitudeSplitte() for splitting it up.

std::string MagnitudeSplitter(std::vector<int> src);
        // Splits a number too big for Stringer to handle into smaller chunks (using VectorSplitter()) of max 3 digits, translate them with Stringer() and appends the right magnitude.

std::vector<int> VectorSplitter(std::vector<int> &V);
        // Splits the number by the hundreds.
        // Ex:  12004 is split into 12 and 004.
        //      1222333 is split into 1 and 222333.
        //      111222 is split into 111 and 222.
// ######################################################

// THESE FUNCTIONS ARE HELPER FUNCTIONS TO Stringer().
std::string units(std::vector<int> src);
        // Translate the singe digits numbers.

std::string decine(std::vector<int> src);
        // Transalte the double digits numbers.
        // ( Considered the structure of the vocabular vectors, i'm considering merging both units and decine into the same function.)

std::string hundreds(std::vector<int> src);
        // Transalte the triple digits numbers.
// #############################################################

// TOOLS.
void FlipVector(std::vector<int>& V) {
        std::reverse(V.begin(), V.end());
}

void PrintVector(std::vector<int> v) {
        int len = v.size();
        for (int i  = 0; i < len; ++i) { std::cout << v[i];}
        std::cout << std::endl;
}

std::vector<int> Splitter(int src_num) {
        // Convert the input number into a vector.
        std::vector<int> v_Digits;
        if (src_num == 0) {
                v_Digits.push_back(0);
                return v_Digits;
        }
        else {
                while(src_num >= 10) {
                        v_Digits.push_back(src_num%10);
                        src_num /= 10;
                }
                v_Digits.push_back(src_num);
                return v_Digits;
        }
}
// ###################################################

std::string units(std::vector<int> src) {
        std::string uni_str = first_twenty_vocabular[src[0]];
        return uni_str;
}

std::string decine(std::vector<int> src) {
        std::string dec_str = "";
        if (src[0] == 0) {
                if (src[1] == 0) { return dec_str; }
                else {
                        dec_str.append(first_twenty_vocabular[src[1]]);
                        return dec_str;
                }
        }
        else if (src[0] == 1) {
                dec_str.append(first_twenty_vocabular[10+src[1]]);
                return dec_str;
        }
        else {
                dec_str.append(decine_vocabular[src[0]-2]);
                if(src[1] == 0) { return dec_str; }
                else {
                        dec_str.append(first_twenty_vocabular[src[1]]);
                        return dec_str;
                }
        }
}

std::string hundreds(std::vector<int> src) {
        std::string hundred_string = "";

        if(src[0] == 0) {
                std::vector<int> dec_vec= {src[1],src[2]};
                std::string dec_str = decine(dec_vec);
                hundred_string.append(dec_str);
                return hundred_string;
        }
        else {
                hundred_string.append(first_twenty_vocabular[src[0]]);
                hundred_string.append("hundred ");
                std::vector<int> dec_vec= {src[1],src[2]};
                std::string dec_str = decine(dec_vec);
                hundred_string.append(dec_str);
                return hundred_string;
        }
}


std::string Stringer(std::vector<int> src) {
        std::string num_string = "";
        int len = src.size();
        if( src[0] < 0 ) {
                num_string.append("minus ");
                for (int i = 0; i < len; ++i) { src[i] = -i; }
        }

        if (len == 1) {
                std::string add = units(src);
                num_string.append(add);
        } else if (len == 2) {
                std::string add = decine(src);
                num_string.append(add);
        } else if (len == 3) {
                std::string add = hundreds(src);
                num_string.append(add);
        } else {
                return MagnitudeSplitter(src);
        }
        return num_string;
}

std::vector<int> VectorSplitter(std::vector<int> &V){
        std::vector<int> first_digits;
        int len = V.size();
        if (len%3 == 0) {
                for(int i = 0; i < 3; ++i) {
                        first_digits.push_back(V[0]);
                        FlipVector(V);
                        V.pop_back();
                        FlipVector(V);
                }

        } else {
                for( int i = 0; i < (len-(3*(len/3))); ++i) {
                        first_digits.push_back(V[0]);
                        FlipVector(V);
                        V.pop_back();
                        FlipVector(V);
                }
        }

        return first_digits;
}


std::string MagnitudeSplitter(std::vector<int> src) {
        int len = src.size();
        std::string tot_str = "";

        if (len == 3) {
                return Stringer(src);
        } else {
                std::vector<int> first_digits = VectorSplitter(src);
                int new_len = first_digits.size();
                std::string add = Stringer(first_digits);

                int sum_values = 0;    // To know if there are only zeroes.
                for ( int i = 0; i < new_len; ++i) {
                        sum_values += first_digits[i];
                }

                tot_str.append(add);
                if (sum_values) {
                        // Appends the right magnitude only if the relative chunk is significative ( with something in it).
                        tot_str.append(magnitude_vocabular[(len-1)/3]);
                }

                std::string return_str = Stringer(src);
                tot_str.append(return_str);
        }
        return tot_str;
}

int main() {
        std::cout << "insert your number: "; int x; std::cin >> x;
        std::vector<int> v_result = Splitter(x); // Face down, ass up.
        FlipVector(v_result);                    // Face up, ass down (poor boy)
        PrintVector(v_result);
        std::cout << Stringer(v_result) << std::endl;


}
\$\endgroup\$
  • \$\begingroup\$ Move everything into a class or a couple of classes, for starters. If you're not looking for crazy inline only efficiency, separate function declarations from definitions. That will help with the spaghetti. \$\endgroup\$ – sunny Jul 15 '15 at 19:06
  • 1
    \$\begingroup\$ BTW: forty not fourty \$\endgroup\$ – JS1 Jul 15 '15 at 20:14
  • \$\begingroup\$ @JS1 ops, good thing i noticed the "fifthteen" before posting \$\endgroup\$ – WhiteEyeTree Jul 16 '15 at 7:04
5
\$\begingroup\$

From the top down, first, given what you want to do, your function had better look like this:

std::string toEnglish(int x);

Your Stringer() function sort of does this, but with a really weird name. In fact, all your functions have really weird names. Splitting into a vector of digits doesn't seem particularly useful to the problem at hand either. And your negative check is definitely wrong:

for (int i = 0; i < len; ++i) { src[i] = -i; }

That'll just overwrite your src with values that had nothing to do with it.

The rest of Stringer()'s logic is really confusing as well. So we have:

if (len == 1) {
    std::string add = units(src);
    num_string.append(add);
} else if (len == 2) {
    std::string add = decine(src);
    num_string.append(add);
} else if (len == 3) {
    std::string add = hundreds(src);
    num_string.append(add);
} else {
    return MagnitudeSplitter(src);
}
return num_string;

Which would be exactly equivalent to:

switch (len) {
case 1: return units(src);
case 2: return decine(src);
case 3: return hundreds(src);
default: return MagnitudeSplitter(src);
}

MagnitudeSplitter then checks for the length being 3, which we know will never happen. But really, why are these different cases at all?

On top of that, you're copying your objects at every point instead of taking by reference to const. I find it very hard to follow your decine and hunrdeds functions. I am not sure if they're correct. I think the best code review I could give you is...


Let's start over. With English number words, the way to do it is to divide the number into blocks of 3 and do each one. So I would expect a helper function like;

/*
 Given a number 1-999, convert it to English.
 Examples:
      2 -> "two"
      127 -> "one hundred twenty seven"
 */
std::string blockToEnglish(int x);

Which we could use like:

std::string toEnglish(int x) {
    if (x == 0) return "zero";

    std::vector<int> blocks;
    while (x > 0) {
        blocks.push_back(x % 1000);
        x /= 1000;
    }

    std::vector<std::string> block_words;
    for (size_t i = 0; i != blocks.end(); ++i) {
        if (blocks[i]) {
            block_words.append(blockToEnglish(blocks[i]));                
        }
    }

    // TODO as exercise: combine block_words into one
    // string with the "millions", etc separators
    // as well as handle negatives

}
\$\endgroup\$
  • \$\begingroup\$ Thanks, the decine and hundred functions are mean to work with vectors of size 2 and 3 respectively (that's why i used the checks in stringer). For example, we look at decine (since hundreds then uses it again ): if the first digit is zero in the 2 digit vector then we have a unit, that's for cases like 109, where 1 gets registered as hundred and then 09 is passed as a decine to the respective function. Then there is another check to see if you have the "teens" or a higher number. \$\endgroup\$ – WhiteEyeTree Jul 16 '15 at 9:08
  • \$\begingroup\$ More over, i don't know why that negative check was still there, i knew it was bogus code to begin with. Anyhow many many thanks, i'll try to implement every change you suggested and rewrite everything from scratch. Cherrs, happy coding! \$\endgroup\$ – WhiteEyeTree Jul 16 '15 at 9:27
3
\$\begingroup\$

Be consistent

std::string Stringer(std::vector<int> src);    
std::string MagnitudeSplitter(std::vector<int> src);
std::vector<int> VectorSplitter(std::vector<int> &V);

Sometimes you start function names with a lowercase letter and sometimes you start with an uppercase letter. Pick one and stick to it. I'd recommend lowercase for these, as it's more common. Uppercase is usually used for class names, not function names or parameters.

A side issue is that functions are usually given verb names like convert and split. It's classes that are given noun names like Stringer or Splitter.

One reason why people like me find this confusing is that there is one kind of function that has noun names and that's the constructor for an object. So when I see these used, it makes me think that you are calling constructors.

One statement per line

        std::cout << "insert your number: "; int x; std::cin >> x;

As a general rule, try to avoid layering multiple statements on a single line. This just makes it harder to follow the logic.

        std::cout << "Enter your number: ";

        int x;
        std::cin >> x;

Also, try to break up your code into paragraphs so people know what goes with what.

Avoid logic in main

        std::vector<int> v_result = Splitter(x);
        FlipVector(v_result);
        PrintVector(v_result);
        std::cout << Stringer(v_result) << std::endl;

Here, you convert the int into a vector, then reverse it, then print it, then output the result of converting it to a string. If the purpose of your code is to convert an int into a string representation, why not just do that?

       std::cout << convertToString(x) << std::endl;

Then inside convertToString, you can do your splits and reverses if need be.

Now if you want to reuse the code in another program, you don't have to copy from main to make it work. You just pass the function your initial value and let it worry about the details.

Use classes

It would be better to use a class for this. You could initialize the class with your int value and then do things like output the vector or the string representations as desired. That way you'd still have access to the internal details if you wanted, but you aren't forcing your clients to know them if they don't want to do so.

No need to else after you return

        if (src_num == 0) {
                v_Digits.push_back(0);
                return v_Digits;
        }
        else {

Since you return at the end of the if block, you don't need that else. You can omit it and have less indent for the remainder of that section.

Another way to handle things in that specific function would be to move the return statements out of the if and else blocks. Then you could just have one return at the very end of the function.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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