4
\$\begingroup\$

I've recently started studying C++ (from internet) Made my very first program, calculator. It works, but i'd like to know how could i make this more "advanced"?

#include <iostream>
using namespace std;

int pluss(int num1, int num2)
{
    int answer = num1 + num2;
    cout << num1 << " + " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
    return 0;
}

int Minus(int num1, int num2)
{
    int answer = num1 - num2;
    cout << num1 << " - " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
    return 0;
}

int devide(double num1, double num2)
{
    double answer = num1 / num2;
    cout << num1 << " / " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
    return 0;
}

int multiply(double num1, double num2)
{
    double answer = num1 * num2;
    cout << num1 << " * " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
    return 0;
}

int main()
{
    int begin;
    cout << "----------------------------------------" << endl;
    cout << "-----Welcome to C++ Based Calculator!---" << endl;
    cout << "-1=Plus--2=Minus--3=devide--4=multiply--" << endl;
    cout << "----------------------------------------" << endl;
    cin >> begin;

    if (begin == 1) {
        system("CLS");
        int x;
        int y;
        cout << "Give me a first number" << endl;
        cin >> x;
        system("CLS");
        cout << "Give me a second number" << endl;
        cin >> y;
        pluss(x, y);
        return main();
    }
    else if (begin == 2) {
        system("CLS");
        int x;
        int y;
        cout << "Give me a first number" << endl;
        cin >> x;
        system("CLS");
        cout << "Give me a second number" << endl;
        cin >> y;
        Minus(x, y);
        return main();
    }
    else if (begin == 3) {
        system("CLS");
        double x;
        double y;
        cout << "Give me a first number" << endl;
        cin >> x;
        system("CLS");
        cout << "Give me a second number" << endl;
        cin >> y;
        devide(x, y);
        return main();
    }
    else if (begin == 4) {
        system("CLS");
        double x;
        double y;
        cout << "Give me a first number" << endl;
        cin >> x;
        system("CLS");
        cout << "Give me a second number" << endl;
        cin >> y;
        multiply(x, y);
        return main();
    }
    else {
        system("CLS");
    }

    system("pause");
    return 0;
}
\$\endgroup\$
1

3 Answers 3

1
\$\begingroup\$

This is a nice first project. Since you are a beginner in C++, I'm not going to worry about the using namespace std and a couple other details.

Naming

Your functions follow an inconsistent naming style and are not spelled correctly:

int pluss(int num1, int num2);

int Minus(int num1, int num2);

int devide(double num1, double num2);

int multiply(double num1, double num2);

These would be better named

int plus(int num1, int num2);

int minus(int num1, int num2);

int divide(double num1, double num2);

int multiply(double num1, double num2);

Consider using void functions

Your functions all return an int. It's not very clear what the return value would mean, so I was puzzled over that for a bit, but then I noticed that they all returned 0:

int pluss(int num1, int num2)
{
    ...
    return 0;
}

It seems that you are copying the idea of int main(), where main returns an error code if something went wrong, but there's no way for something to go wrong in these functions barring an exception, so just make these functions void:

void pluss(int num1, int num2)
{
    int answer = num1 + num2;
    cout << num1 << " + " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
}

On the other hand, the names of your functions seem to suggest that they should return the result of the operation, in which case you might do this:

int plus(int num1, int num2)
{
    return num1 + num2;
}

But don't do this. This is simply re-inventing the + operator.

System dependency

Your code uses system() all over the place. system is not cross platform, and it is better to make the code as cross-platform as possible. You really don't need to use system("CLS") everywhere; people using a console don't expect their console to clear old messages. Furthermore, system("pause") isn't necessary; the console waits whenever your program needs to obtain input.

Switch

In your main function, you have this:

if (begin == 1) {
    ...
}
else if (begin == 2) {
    ...
}
else if (begin == 3) {
    ...
}
else if (begin == 4) {
    ...
}
else {
    ...
}

This is basically a switch statement; you could just code that like so:

switch (begin) {
case 1: ...
case 2: ...
case 3: ...
case 4: ...
default: ...
}

Why only add and subtract ints?

If you think it's a good idea to be able to divide and multiply doubles, why not have the same thing for add and subtract? That's definitely something I would expect if I were using your calculator.

Functions should only do "one" thing

Your functions do multiple things:

void multiply(double num1, double num2)
{
    double answer = num1 * num2;
    cout << num1 << " * " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
}

Notice how this function is less of a mathematical function, and more of a "perform this task". The task you are having it perform is to "display the result of multiplication, then clear the screen afterwards". That's most definitely multiple things. It would be better to make the function this:

void showMultiply(double num1, double num2)
{
    double answer = num1 * num2;
    cout << num1 << " * " << num2 << " = " << answer << endl;
}

But if you were to do this with all your functions, suddenly they are all almost the same:

void showPlus(double num1, double num2)
{
    double answer = num1 + num2;
    cout << num1 << " + " << num2 << " = " << answer << endl;
}

void showMinus(double num1, double num2)
{
    double answer = num1 - num2;
    cout << num1 << " - " << num2 << " = " << answer << endl;
}

void showMultiply(double num1, double num2)
{
    double answer = num1 * num2;
    cout << num1 << " * " << num2 << " = " << answer << endl;
}

void showDivide(double num1, double num2)
{
    double answer = num1 / num2;
    cout << num1 << " / " << num2 << " = " << answer << endl;
}

We can collapse these down to result in less duplication:

void showCalculation(double num1, double num2, double answer, char operator)
{
    cout << num1 << ' ' << operator << ' ' << num2 << " = " << answer << endl;
}

Now when we use it, it would look like this:

double answer = num1 + num2;
showCalculation(num1, num2, answer, '+');
...

However, since this function is only one line of code, I personally wouldn't use the function.

Code duplication

Notice how each block in the if statements look almost identical:

if (begin == 1) {
    system("CLS");
    int x;
    int y;
    cout << "Give me a first number" << endl;
    cin >> x;
    system("CLS");
    cout << "Give me a second number" << endl;
    cin >> y;
    pluss(x, y);
    return main();
}
else if (begin == 2) {
    system("CLS");
    int x;
    int y;
    cout << "Give me a first number" << endl;
    cin >> x;
    system("CLS");
    cout << "Give me a second number" << endl;
    cin >> y;
    Minus(x, y);
    return main();
}
else if (begin == 3) {
    system("CLS");
    double x;
    double y;
    cout << "Give me a first number" << endl;
    cin >> x;
    system("CLS");
    cout << "Give me a second number" << endl;
    cin >> y;
    devide(x, y);
    return main();
}
else if (begin == 4) {
    system("CLS");
    double x;
    double y;
    cout << "Give me a first number" << endl;
    cin >> x;
    system("CLS");
    cout << "Give me a second number" << endl;
    cin >> y;
    multiply(x, y);
    return main();
}

This is a sign that you should write functions to break it up. This is what I'd do:

double promptForInput(const string &message)
{
    double result;
    cout << message << endl;
    cin >> result;
    return result;
}

...

if (begin == 1) {
    double num1 = promptForInput("Give me the first number");
    double num2 = promptForInput("Give me the second number");
    double result = num1 + num2;
    showCalculation(num1, num2, result, '+');
}
// and so on

Calling main() is undefined behaviour

It is not legal to call main() in your C++ code. You are using this to loop; you should instead learn how to use loops. This is one example of how to do so:

int main()
{
    do {
        int begin;
        cout << "----------------------------------------" << endl;
        cout << "-----Welcome to C++ Based Calculator!---" << endl;
        cout << "-1=Plus--2=Minus--3=devide--4=multiply--" << endl;
        cout << "----------------------------------------" << endl;
        cin >> begin;

        if (begin == 1) {
            ...
        }
        ...
        else {
            break;
        }
    } while (true);

    return 0;
}

Use constants

Since you are using numbers to represent what choice the user made, I strongly recommend you store them as constants:

constexpr int PLUS = 1;
constexpr int MINUS = 2;
constexpr int DIVIDE = 3;
constexpr int MULTIPLY = 4;

Final result

After apply all my recommendations, this is my final result:

#include <string>
#include <iostream>

using namespace std;

double promptForInput(const string &message)
{
    double result;
    cout << message << endl;
    cin >> result;
    return result;
}

constexpr int PLUS = 1;
constexpr int MINUS = 2;
constexpr int DIVIDE = 3;
constexpr int MULTIPLY = 4;

int main()
{
    do {
        int choice; // choice is a better name for this
        cout << "----------------------------------------" << endl;
        cout << "-----Welcome to C++ Based Calculator!---" << endl;
        cout << "-1=Plus--2=Minus--3=Divide--4=Multiply--" << endl;
        cout << "----------------------------------------" << endl;
        cin >> choice;

        double num1 = promptForInput("Give me the first number");
        double num2 = promptForInput("Give me the second number");

        double result;
        char operatorName;

        switch (choice) {
        case PLUS:
            result = num1 + num2;
            operatorName = '+';
            break;
        case MINUS:
            result = num1 - num2;
            operatorName = '-';
            break;
        case DIVIDE:
            result = num1 / num2;
            operatorName = '/';
            break;
        case MULTIPLY:
            result = num1 * num2;
            operatorName = '*';
            break;
        default:
            cout << "Some message thanking the user for using the calculator (optional). " << endl;
            return 0; // to exit main
        }

        cout << num1 << ' ' << operatorName << ' ' << num2 << " = " << result << endl;
    } while (true);
}

Going further

If you wanted to make this "even better", this is how I'd fully implement this:

#include <iostream>
#include <sstream>
#include <string>

void printOptions() {
    std::cout << "------------------------------------------" << std::endl;
    std::cout << "----- Welcome to C++ Based Calculator! ---" << std::endl;
    std::cout << "- Plus:     +                          ---" << std::endl;
    std::cout << "- Minus:    -                          ---" << std::endl;
    std::cout << "- Divide:   /                          ---" << std::endl;
    std::cout << "- Multiply: *                          ---" << std::endl;
    std::cout << "- Quit:     Enter                      ---" << std::endl;
    std::cout << "------------------------------------------" << std::endl;
}

constexpr char QUIT = '\0';

char menu() {
    printOptions();

    std::string input;
    std::getline(std::cin, input);

    if (input.size() != 1) {
        return QUIT;
    }
    return input[0];
}

double promptForInput(const std::string &message) {
    std::cout << message << std::endl;

    double result;
    std::istringstream input;
    do {
        std::string line;
        std::getline(std::cin, line);
        input.str(line);

        input >> result;

        if (!input) {
            std::cout << "Please input a valid number" << std::endl;
        }
    } while (!input);

    return result;
}

double performOperation(double num1, double num2, char choice) {
    switch (choice) {
    case '+': return num1 + num2;
    case '-': return num1 - num2;
    case '*': return num1 * num2;
    case '/': return num1 / num2;
    }
    std::cerr << "Internal Error" << std::endl;
    std::exit(1);
}

int main() {
    do {
        char choice = menu();
        if (choice == QUIT) return 0; // The user wants to quit

        double num1 = promptForInput("Give me the first number");
        double num2 = promptForInput("Give me the second number");

        double result = performOperation(num1, num2, choice);

        std::cout << num1 << ' ' << choice << ' ' << num2 << " = " << result << std::endl;
    } while (true);
}

Here's what I did differently:

  • I had the user input the actual character for the operation they want to use. This makes my life a little easier, as I can just pass that around. If I didn't want to do this, I'd actually translate the number into the character.
  • I didn't use the using namespace std;. That statement pollutes your namespace with all the symbols from the standard namespace; there's a reason why the standard namespace is only 3 characters long
  • I used std::getline to get the input. This makes it so that the user has to input everything on its own line. I liked this idea better.
  • I moved the switch into its own function; this makes the main function shorter, making it easier to understand.
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Lot I agree with log I disagree with in this review. void rather than int return. Disagree. Use a switch statement (better than if) but still not a good idea; use a function map. Suggesting a loop is good. But then using a do{}while; loop rather than a while{} loop is a bad choice. All uppercase identifiers (for your constants) is a bad idea (all uppercase identifiers should be reserved for Macros). \$\endgroup\$ Commented Nov 23, 2016 at 3:19
  • \$\begingroup\$ @LokiAstari There's a lot I would have done different if the OP wasn't new to C++, such as the function map. I personally prefer do-while loops, and all uppercase constants is what some people do. I figured it was a more common style than my preferred style of kMyConstant. I don't understand why void rather than int return is bad. \$\endgroup\$
    – Justin
    Commented Nov 23, 2016 at 4:33
0
\$\begingroup\$

This is not review but a continuation of @justin.
So please don't vote.

Though I disagree on a few minor points (which I point out in the comment). I think @justin's review covers all the basics.

I just want to show an example that uses a map rather than a switch. Also returning multiple values from a function (as a secondary thing). But I don't want to do a whole review.

#include <iostream>
#include <sstream>
#include <functional>
#include <map>
#include <tuple>

double add(double lhs, double rhs) {return lhs + rhs;}
double sub(double lhs, double rhs) {return lhs - rhs;}
double multiply(double lhs, double rhs) {return lhs * rhs;}
double divide(double lhs, double rhs) {return lhs / rhs;}

using ActionMap = std::map<char, std::function<double(double,double)>>;

ActionMap actionMap {{'+', add}, {'-', sub}, {'*', multiply}, {'/', divide}};

bool validAction(char action)
{
    return actionMap.find(action) != actionMap.end();
}

std::tuple<double, double, char> getValue()
{
    std::cout << "Please provide two numbers and an action (on one line)\n";
    std::string line;

    while(std::getline(std::cin, line))
    {
        std::stringstream lineStream(line);

        double lhs;
        double rhs;
        char   action;
        if ((lineStream >> lhs >> rhs >> action) && validAction(action)) {
            return {lhs, rhs, action};
        }
        std::cout << "Error: I did not find <number> <number> <action>\n"
                  << "Action can be one of: ";
        for(auto kv: actionMap) {
            std::cout << kv.first << " ";
        }
        std::cout << "\n";
    }
    std::cout << "End of input reached\n";
    throw std::runtime_error("Fail");
}

int main()
{    
    while(true)
    {
        double lhs;
        double rhs;
        char   action;
        std::tie(lhs, rhs, action) = getValue();

        std::cout << "Action:  " << lhs << " " << action << " " << rhs
                  << " Result: " << actionMap[action](lhs, rhs)
                  << "\n";
    }
}
\$\endgroup\$
0
\$\begingroup\$

As I'm not a guru yet in C++, here are few pointers

  • Function names should start with lowercase letters e.g Minus() should be minus
  • There is no need to have int has a return type for your function. void is a valid return type and it means return nothing. This would save you the lines of return 0
  • Avoid using namespace std because when you use additional libraries clashes may occur. Instead use qualified names e.g std::cout, std::endl etc
  • I see duplicate codes, DRY up your main function. Make first and second numbers global so this can be read once and it's outside your if statement
  • At this point, I will opt for a switch statement to make your main cleaner
  • To make this robust, you probably have to check for if invalid input was typed by the user
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Just wondering why my answer was downvoted. Please drop comments so I can improve my answer \$\endgroup\$
    – Tolani
    Commented Nov 22, 2016 at 22:25
  • \$\begingroup\$ Your answer isn't bad, but there are a few things that are off about it. For instance, your third bullet point about divide and multiply giving only the integral part is incorrect. Also, advocating for the first and second numbers being global is a bad idea, although it looks like you are saying to just put those outside of the if statements (code would help here). You misuse the word "exceptions" because they actually mean something in C++, so it is worth saying "You should check for invalid input" instead. \$\endgroup\$
    – Justin
    Commented Nov 22, 2016 at 23:00
  • \$\begingroup\$ Don't see any reason for a down vote. \$\endgroup\$ Commented Nov 23, 2016 at 3:11
  • \$\begingroup\$ I have improved my answers based on your suggestions @Justin \$\endgroup\$
    – Tolani
    Commented Nov 23, 2016 at 6:19

Your Answer

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

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