1
\$\begingroup\$
#include <iostream>

using namespace std;

class Math
{
private:
    int answer;
public:
    /*void getAn()
    {
        cout << answer;
    }*/
    int add(int x, int y)
{
    cout << "Add two numbers:\n";
    cin >> x;
    cin >> y;
    answer = x + y;
    return answer;
}
    int sub(int x, int y)
{
    cout << "Subtract one number from the other:\n";
    cin >> x;
    cin >> y;
    answer = x - y;
    return answer;
}
    int multi(int x, int y)
{
    cout << "Multiply two numbers:\n";
    cin >> x;
    cin >> y;
    answer = x * y;
    return answer;
}
    int divi(int x, int y)
{
    cout << "Divide two numbers:\n";
    cin >> x;
    cin >> y;
    answer = x / y;
    return answer;
}
};

int main()
{
    int choice;
    int x,y;
    int learning = 1;
    Math doMath;
    cout << "This is a calculator!\n" << endl;
    while(learning==1)
    {
        //doMath.getAn();
        cout << endl;
        cout << "Menu:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\n5) Quit\n" << endl;
        cout << "Choose your option.\n";
        cin >> choice;
        if(choice==1)
        {
            cout << doMath.add(x,y) << endl;
        }else if(choice==2)
        {
            cout << doMath.sub(x,y) << endl;
        }else if(choice==3)
        {
            cout << doMath.multi(x,y) << endl;
        }else if(choice==4)
        {
            cout << doMath.divi(x,y) << endl;
        }else if(choice==5)
        {
            learning = 0;
        }else
        {
            cout << "THAT WASN'T A CHOICE, YOU LITTLE SHIT!!!" << endl;
            learning = 0;
        }
    }
    return 0;
}
\$\endgroup\$
5
  • 5
    \$\begingroup\$ Welcome. Does this code work as intended? We need to make sure of that. I'd also recommend replacing the one curse word with something else, especially if this is to be a serious application. \$\endgroup\$
    – Jamal
    Nov 25, 2014 at 19:41
  • \$\begingroup\$ @ Jamal you are right, software should never offend users, I wrote that advice in my answer too. \$\endgroup\$
    – Caridorc
    Nov 25, 2014 at 19:49
  • \$\begingroup\$ This is just a practice program I am writing as I am going about learning C++. Nobody is using this except for me @Caridorc \$\endgroup\$
    – Arvus
    Nov 25, 2014 at 19:53
  • 4
    \$\begingroup\$ @Arvus Ok, anyway as a rule of thumb (not only in programming but also in life) limit insults to a minimum. \$\endgroup\$
    – Caridorc
    Nov 25, 2014 at 19:55
  • 2
    \$\begingroup\$ It's also helpful if you can provide a bit of description about the problem that the code is solving. It's not a huge deal in this case because the code is straightforward, but still good practice. \$\endgroup\$
    – Brythan
    Nov 25, 2014 at 20:54

2 Answers 2

10
\$\begingroup\$

Do not insult users

"THAT WASN'T A CHOICE, YOU LITTLE SHIT!!!" is absolutely not acceptable.

  • You are repeating yourself a lot, in programming you should avoid as much as possible code repetition as it leads to bugs and is not easily reusable.

    Instead do this:

    template<typename Function>
    int do_operation(int x, int y,Function operation,string message) {
        cout << message
        cin >> x;
        cin >> y;
        answer = operation(x,y);
        return answer;
    }
    

    You can then do:

    int add(int x,int y) {
        return do_operation(x, y, std::plus<int>(), "Add two numbers:\n");
    }
    
  • I see many many if and elif, it is better to use a switch statement. You may even use an array but perhaps then it would be overkill.

  • learning is quite puzzling as a name. A more standard running is more intuitive.

  • //doMath.getAn(); and

    /*void getAn()
    {
        cout << answer;
    }*/
    

    are really puzzling. Either uncomment them or delete them because commented out code confuses the readers.

  • Use constants for your strings and put them at the top. You can modify them in a simpler and faster way.

    static const string Menu = "Menu:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\n5) Quit\n"
    static const string ErrorMessage = "Invalid choice, enter a valid number."
    

    and then

    cout << Menu << endl;
    cout << "Choose your option.\n";
    

    and

    else {
        cout << ErrorMessage << endl;
        running = 0;
    }
    
  • Do not use while(learning==1) because it is redundant and weird, use the simpler while(learning) (or even better while(running))

  • Do not use using namespace std;, if you are curious to know why, look here
  • Put the first brace on the same line of the function title and indent the function body.

    int add(int x, int y) {
        cout << "Add two numbers:\n";
        cin >> x;
        cin >> y;
        answer = x + y;
        return answer;
    }
    
\$\endgroup\$
7
  • \$\begingroup\$ Please clarify the expression, in your add function: (x,y) => x + y. I don't recognize it. \$\endgroup\$ Nov 25, 2014 at 20:48
  • \$\begingroup\$ @ThomasMatthews I took that sintax from msdn.microsoft.com/en-us/library/bb397687.aspx . It is a lambda expression. \$\endgroup\$
    – Caridorc
    Nov 25, 2014 at 20:50
  • \$\begingroup\$ @Caridorc: That corresponds to C#, not C++. \$\endgroup\$
    – Jamal
    Nov 25, 2014 at 21:03
  • 1
    \$\begingroup\$ The C++ lambda syntax would have been too long. I took the liberty to replace it by the standard functor std::plus<int>(). \$\endgroup\$
    – Morwenn
    Nov 25, 2014 at 21:30
  • 3
    \$\begingroup\$ I can't emphasis enough the point about not insulting the user (even in throw away code). A colleague of mine was fired as a result of a mistake where he forgot to change the string he was using for testing "Dear Rich XXXXXXX" when the code went into production. As a result the banks largest clients got an e-mail with a very unprofessional salutation. \$\endgroup\$ Nov 26, 2014 at 4:21
3
\$\begingroup\$

You don't need to pass x and y into your functions. You are passing everything by value, so you might as well declare them inside the functions. The versions outside the functions are never used. If you use Caridorc's method, you could just

template<typename Function>
int do_operation(Function operation,string message) {
    int x, y;
    cout << message
    cin >> x;
    cin >> y;
    answer = operation(x,y);
    return answer;
}

Your use of endl is a bit odd.

cout << "This is a calculator!\n" << endl;
    cout << endl;
    cout << "Menu:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\n5) Quit\n" << endl;
    cout << "Choose your option.\n";

You get two things from std::endl. First, it issues a line feed in the appropriate fashion for that system. Second, it causes the output buffer to flush. I.e. it takes everything that is currently in the buffer and makes it output. Sometimes people will tell you to use a \n rather than an std::endl to avoid that flush. If you were going to do that in your code, you'd say

std::cout << "This is a calculator!\n\n";
    std::cout << "\nMenu:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\n5) Quit\n\n";
    std::cout << "Choose your option.\n" << std::endl;

That does one flush at the end of the output (and you may not need that, as the std::cin will also force a flush).

Math doMath;

Objects are not verbs. It is not doing anything, so give it a noun name. E.g. math or calculator.

int learning = 1;
while(learning==1)
        learning = 0;

As Caridorc noted, learning is a bad name here. Another issue is that learning only has two possible values. This suggests to me that it is either true or false, so use a true/false variable:

bool isDone = false;
while ( ! isDone )
        isDone = true;

That's much clearer about what it's doing, but the truth is that you don't even need to do that in this case.

while (true)
        break;

That's sufficient for your needs. No extra variable needed. Loop forever until explicitly told to stop by the break; statement. Alternately, you could also use return EXIT_SUCCESS; and return EXIT_FAILURE; instead of break; in your code. You might have to do an #include <cstdlib> to get those defined.

return 0;

You don't actually need to return anything in this case. Just let it fall through and the compiler will automatically return the appropriate value.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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