4
\$\begingroup\$

Program

It takes a numerical input in range [1, 99] and outputs its word equivalent.

Concerns

  • Procedural approach

  • General code style

Code

#include <iostream>

void print1to20(int num)
    {
           switch(num)
           {
           case 1: std::cout << "one"; break;
           case 2: std::cout << "two"; break;
           case 3: std::cout << "three"; break;
           case 4: std::cout << "four"; break;
           case 5: std::cout << "five"; break;
           case 6: std::cout << "six"; break;
           case 7: std::cout << "seven"; break;
           case 8: std::cout << "eight"; break;
           case 9: std::cout << "nine"; break;
           case 10: std::cout << "ten"; break;
           case 11: std::cout << "eleven"; break;
           case 12: std::cout << "twelve"; break;
           case 13: std::cout << "thirteen"; break;
           case 14: std::cout << "fourteen"; break;
           case 15: std::cout << "fifteen"; break;
           case 16: std::cout << "sixteen"; break;
           case 17: std::cout << "seventeen"; break;
           case 18: std::cout << "eighteen"; break;
           case 19: std::cout << "nineteen"; break;
           case 20: std::cout << "twenty"; break;
           default: std::cout << "1 to 20 switch error";
           }
    }

void printTens(int num)
    {
           switch(num)
           {
           case 20: std::cout << "twenty"; break;
           case 30: std::cout << "thirty"; break;
           case 40: std::cout << "fourty"; break;
           case 50: std::cout << "fifty"; break;
           case 60: std::cout << "sixty"; break;
           case 70: std::cout << "seventy"; break;
           case 80: std::cout << "eighty"; break;
           case 90: std::cout << "ninety"; break;
           default: std::cout << "tens switch error";
           }
    }

int main()
{
    int num;
    do
    {
    std::cout << "Insert a number 1-99: ";
    std::cin >> num;
    } while(num < 1 || num > 99);

    int num_a, num_b; // num = (a)(b);
    num_a = num - num%10; 
    num_b = num%10;

    std::cout << "Your number is ";

    if(num <= 20)
        print1to20(num);
    else
    {
        printTens(num_a);
        std::cout << ' ';
        print1to20(num_b);
    }

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ @Zeta, I've gutted the question. Alien, please see if you're ok with it. Also, please try to follow the style I've used (e.g. describe the program, your concerns, and put code). People will put efficient ways of solving the problem, it might not be what you expect. \$\endgroup\$ – Incomputable Sep 15 '17 at 18:58
  • \$\begingroup\$ I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Sep 15 '17 at 20:22
5
\$\begingroup\$
  1. The code is broken. If a number is greater than 20 and ends with 0, print1to20(num_b) will not do what it should.

  2. Don't mix the logic with the output. I'd change the return type of your functions to std::string. It'll make the code more testable and reusable.

  3. Don't just print something to std::cout in a case of an error. Throw an exception instead.

  4. Declare each variable as late as possible:

    int num_a, num_b; // num = (a)(b);
    num_a = num - num%10; 
    num_b = num%10;
    

    should be

    int num_a = ...;
    int num_b = ...;
    
  5. Use more descriptive names. I have no clue what num_a mean. What is a? Something like first_digit and second_digit would be better (well the first one is not exactly a digit, so you can come with even better names).

  6. There's no need to return 0 from the main function explicitly (sure, you can if you want to).

  7. Write proper automated tests for your code to reduce the number of bugs (your bug is very likely to get caught by tests). In fact, you can check all possible valid inputs. There're only 100 of them.

  8. There's nothing wrong in having stand-alone functions. There's no entity here, so I don't see the point of modeling this problem in terms of objects. A function from an int to its string representation is just a function.

\$\endgroup\$
2
\$\begingroup\$

This code is only needed if you are nto below 20

int num_a, num_b; // num = (a)(b);
num_a = num - num%10; 
num_b = num%10;

So I would re-factor so that is only used then.

if(num <= 20) {
    std::cout << print1to20(num);
}
else {
    // Note I changed the input here (see below)
    std::cout << printTens(num / 10);
    // I would handle the space and check of zero as part of the pritn functionality.
    std::cout << print1to20(num % 10); 
}

Actually the whole code printing part should be its own function. Also since your functions are called print*() not getPrintableString*() I would pass the stream you want to print on as one of the arguments.

void printNumber(std::ostream& out, int num) {
    if(num < 20) {
        print1to20(out, num);
    }
    else {
        printTens(out, num / 10);
        print1to20(out, num % 10); 
    }
}

Rather than having a big switch I would use a container and just print the content of a location:

void print1to20(st::ostream& out, int num)
{
     static std::string value[] = {"", "one", "two", "three", ..... "nineteen" };

     // This is an unchecked function. It assumes the value is in range.
     // This is standard practice for C++. If you need to have a version
     // version that checks the range create that as a seprate function
     // so that good developers are not constrained by bad ones.
     out << value[num] << " ";
}
void print1to20Checked(st::ostream& out, int num)
{
    if (num < 1 || 20 <= num) {
        throw std::out_of_range("print1to20Checked");
     }
     print1to20Checked(out, num);
}

You can apply the same logic to the tens function.

void printTens(std::ostream& out, int num)
{
    static std::string value[] = {"", "", "twenty", "thirty", .... "ninty"};
    out << value[num] << " ";
}
\$\endgroup\$
-2
\$\begingroup\$

How I re-wrote my code:

#include <iostream>
#include <string>

std::string print1to20(int num)
    {
           switch(num)
           {
           case 1: return "one";
           case 2: return "two";
           case 3: return "three";
           case 4: return "four";
           case 5: return "five";
           case 6: return "six";
           case 7: return "seven";
           case 8: return "eight";
           case 9: return "nine";
           case 10: return "ten";
           case 11: return "eleven";
           case 12: return "twelve";
           case 13: return "thirteen";
           case 14: return "fourteen";
           case 15: return "fifteen";
           case 16: return "sixteen";
           case 17: return "seventeen";
           case 18: return "eighteen";
           case 19: return "nineteen";
           case 20: return "twenty";
           }
           return "Error!";
    }

std::string printTens(int num)
    {
           switch(num)
           {
           case 20: return "twenty";
           case 30: return "thirty";
           case 40: return "fourty";
           case 50: return "fifty";
           case 60: return "sixty";
           case 70: return "seventy";
           case 80: return "eighty";
           case 90: return "ninety";
           }
           return "Error!";
    }

int main()
{
    int num;
    do
    {
    std::cout << "Insert a number 1-99: ";
    std::cin >> num;
    } while(num < 1 || num > 99);

    int tens = num - num%10; 
    int ones = num%10;

    std::cout << "Your number is ";

    if(num <= 20)
        std::cout << print1to20(num);
    else
    {
        std::cout << printTens(tens);
        if(num%10 != 0)
            std::cout << ' ' << print1to20(ones);
    }

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Redundant "twenty" case. Also, you could handle 13-19 as 3...9 + check for double t's + "teen" . \$\endgroup\$ – einpoklum Sep 15 '17 at 21:17
  • 4
    \$\begingroup\$ A code review will generally be more useful if it includes at least some review of the code, not just how his do the job. I realize this is posted as an answer to your own question, but it's still posted as an answer, so it should really attempt to answer the question (i.e., provide a real review of the code). \$\endgroup\$ – Jerry Coffin Sep 16 '17 at 4:36

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.