I'm relatively new to programming, but am really enjoying it. I have taken a couple of classes but mostly code for fun (for now). I decided to make a program implementing some basic functions, switch statements, and user input. Please leave any feedback on how to make this program better.

I understand that I am not actually using the defined functions (add, subtract, etc.), but I put them in there with the intent of using them, I just don't know how.

#include <iostream>
using namespace std;

int add (int x, int y){
return x+y;
};

int divide (int x, int y){
return x/y;
}

int multiply (int x, int y){
return x*y;
}

int subtract (int x, int y){
return x-y;
}


int main(){

int n1;
int n2;
int user14 = 0;

SomeLine:
cout << "Enter your 2 numbers: "<< endl;
cin >> n1;
cin >> n2;


cout << "Ok, now what do you want to do with those numbers? "<< endl;

cout << "1) Add: " << endl;
cout << "2) Divide: "<<endl;
cout << "3) Multiply: "<< endl;
cout << "4) Subtration: "<< endl;

cin >> user14;

switch (user14)
{
    case 1:
    cout << n1+n2 << endl;
        break;
    case 2:
    cout << n1/n2 << endl;
        break;
    case 3:
    cout << n1*n2<< endl;
        break;
    case 4:
    cout << n1-n2 << endl;
        break;

}

char userchoice;
cout << "Would you like to perform any other operations? y/n "<< endl;

cin >> userchoice;

if (userchoice=='y'){
    goto SomeLine;
}
else if(userchoice=='n'){
    goto Exit;
}

Exit:
return 0;

};
New contributor
braisedbeefcake is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
  • 1
    x/y; should make sure that y is not 0. Instead of using goto, a do.. while loop is better – Sailesh D Dec 6 at 6:50
  • Your program doesn't call the functions named multiply, divide, add and subtract. :) – Kaz Dec 6 at 14:30
  • Minor thing: If the code in the question doesn't look like (i.e. isn't indented the same as) your actual code, instead of manually indenting every line, try pasting the block in, selecting the entire block, and hitting Ctrl+K (Cmd+K on Mac). – Nic Hartley Dec 6 at 18:37

Welcome! Here is some feedback, in no particular order.

Avoid using namespace std

This advice is often repeated for good reason. See also: code guidelines SF.6. It's useful when you're learning the C++ standard library to know what belongs in the standard library and what doesn't.

cout << "Enter your 2 numbers: "<< endl;

Becomes

std::cout << "Enter your 2 numbers: "<< std::endl;

Consistent spacing

Maybe this is caused by the formatting on Stack Exchange. There are many online style guides, I would choose one that is most readable to you and your peers and stick with it throughout the source code.

int add (int x, int y){
return x+y;
};

Becomes

int add(int x, int y) {
    return x + y;
};

Semicolons

Functions don't need semicolons at the end (main & add). This is only used for function prototypes.

int add(int x, int y) {
    return x + y;
};

Becomes

int add(int x, int y) {
    return x + y;
}

Avoid std::endl

Unless you need to flush the stream, using std::endl can be replaced with a newline character.

std::cout << "Enter your 2 numbers: "<< std::endl;

Becomes

std::cout << "Enter your 2 numbers:\n";

Combine multiple std::cout and std::cin calls.

This is just my preference. It reduces the amount of code I have to write.

std::cin >> n1;
std::cin >> n2;

Becomes

std::cin >> n1 >> n2;

And

std::cout << "1) Add: " << std::endl;
std::cout << "2) Divide: "<< std::endl;
std::cout << "3) Multiply: "<< std::endl;
std::cout << "4) Subtration: "<< std::endl;

Becomes

std::cout << "1) Add:\n2) Divide:\n3) Multiply:\n4) Subtration:\n";

In the above example, there are many other ways to do it. For example, you can use multi-line strings, adding the strings, and so on.

Make use of your functions

You define the add, divide, multiply and subtract functions, but you don't call them. Here is how you would do that:

std::cout << n1 + n2 << '\n';

Becomes

std::cout << add(n1, n2) << '\n';

It's as simple as that!

In complex applications, splitting things into functions makes the code easier to work with. For example, say you wanted the "add" operation to have saturating arithmetic (doesn't overflow). It's easier to add that logic to a separate function rather than inserting it into a long block of code.

Or, if you want to handle a division-by-zero case.

Don't use goto

While goto is useful in a few cases, it's not particularly useful here. Instead you can use a do-while loop.

SomeLine:

// code

if (userchoice == 'y'){
    goto SomeLine;
}
else if(userchoice == 'n'){
    goto Exit;
}

Exit:
return 0;

Becomes

do {
     // code
} while (userchoice == 'y');

return 0;

Variable naming

The variables x and y are used in the function signatures. You can also use those names in main. I'd prefer that over n1 and n2. Likewise, user14 is vague (do we change it to user15 if we include another operation?). Something like operation or op isn't vague. userchoice is a bit long, exit is shorter and clearly shows its purpose.

const

Make use of the const keyword where it makes sense. For example, in add, we intend for the x and y variables to never themselves be modified. So we can declare them as const, and the compiler will give us an error if we accidentally change their values.

This isn't particularly useful anything other than readability here, but can be useful if you get into more advanced topics, such as const references.

The functions add, divide, multiply, and subtract can be constexpr. The compiler may already evaluate these at compile time, but using constexpr marks them as such (broadly speaking).

Move variables to the lowest scope

Now that we've gotten rid of the gotos, we can move x, y, and op to a lower scope. They can also all be declared on the same line.

Consider edge cases

The following are a few example cases where you may wish to add extra handling -- or leave as it currently is.

  • What if the user enters a letter where you're expecting an integer?
  • What if the user enters an operation number outside of the range?
  • What about possible signed integer overflow (undefined behavior)?
  • What about division-by-zero?
  • What if the input stream is no longer valid after user input?
  • ...

Conclusion

Here is the code I ended up with:

#include <iostream>

constexpr int add(const int x, const int y) {
    return x + y;
}

constexpr int divide(const int x, const int y) {
    return x / y;
}

constexpr int multiply(const int x, const int y) {
    return x * y;
}

constexpr int subtract(const int x, const int y) {
    return x - y;
}


int main() {
    char exit;

    do {
        int x, y, op;

        std::cout << "Enter your 2 numbers:\n";

        std::cin >> x >> y;

        std::cout << "Ok, now what do you want to do with those numbers?\n";
        std::cout << "1) Add:\n2) Divide:\n3) Multiply:\n4) Subtration:\n";

        std::cin >> op;

        switch (op) {
            case 1:
                std::cout << add(x, y) << '\n';
                break;
            case 2:
                std::cout << divide(x, y) << '\n';
                break;
            case 3:
                std::cout << multiply(x, y) << '\n';
                break;
            case 4:
                std::cout << subtract(x, y) << '\n';
                break;
        }

        std::cout << "Would you like to perform any other operations? y/n\n";

        std::cin >> exit;
    } while (exit == 'y');

    return 0;
}

Hopefully this has been helpful. I hope you continue to enjoy programming as much as we all do!

  • 1
    Wow! Such great feedback. The indenting was due to formatting, like you said. I will get in the habit of avoiding using namespace std. I’ve taken 3 coding classes (c++ 1 and 2, and data structures) and all my instructors have used it. Poor practice I suppose. I am familiar with do while loops and am not entirely sure why I chose otherwise. Getting in the habit of declaring variables in single lines seems to be very effective as well. I look forward to posting more of my programs as I progress and hopefully return as much useful feedback as I have. Thank you! – braisedbeefcake Dec 6 at 8:06
  • Minor nit: add() is a function, not a "method" (by which I assume you mean a member function, since C++ doesn't define the term). – Toby Speight Dec 6 at 9:52
  • 1
    Could you elaborate on your point about avoiding std::endl and instead using \n? Is it just to type less or there are important differences to be taken in account? Thanks – Sembei Norimaki Dec 6 at 12:23
  • 1
    It's not a header file, so your using namespace std guideline does not apply. I'd tend to go the other way on multiple calls to std::cout << as merging them onto one line is harder to maintain and harder to read and spot the spelling error. Putting constexpr on a function that is only used in one file to process user input is probably YAGNI. – Pete Kirkham Dec 6 at 12:39
  • 1
    @PeteKirkham It's still going to bite you in the ass if you get used to doing it. Dive into ODR and ADL stuff -- it's "fun". It's not just about not using namespace std, but always qualifying calls. – Nic Hartley Dec 6 at 18:39

One item missing from the other replies is that you should avoid mixing output and computations.

Instead of

        switch (op) {
        case 1:
            std::cout << add(x, y) << '\n';
            break;
        case 2:
            std::cout << divide(x, y) << '\n';
            break;
        case 3:
            std::cout << multiply(x, y) << '\n';
            break;
        case 4:
            std::cout << subtract(x, y) << '\n';
            break;
    }

Use:

    int res;
    switch (op) {
        case 1:
            res = add(x, y);
            break;
        case 2:
            res = divide(x, y);
            break;
        case 3:
            res = multiply(x, y);
            break;
        case 4:
            res = divide(x, y);
            break;
    }
    std::cout << res << '\n';

Or turn this into a separate function:

   int computation(int op, int x, y) {
    switch (op) {
        case 1:
            return add(x, y);
        case 2:
            return divide(x, y);
        case 3:
            return multiply(x, y);
        case 4:
            return divide(x, y);
     }
   }
   ...
   int res=computation(op, x, y);

There are two reasons:

  • Easier to debug.
  • Easier to change the formatting of the output in a common way.

Welcome on Code Review!


Don't use using namespace std;

Putting using namespace std at the top of every program is a bad practice that can cause a lot of problems that are easy to avoid, most people are used to the prefix, and three letters aren't that hard to type.

  • Although it's "safe" to use it in some place (e.g. implementation files), as long as you aren't familiar with the features of c ++, try to avoid it.
  • It led to a world of name collisions. (best case)
  • It's source of silent errors and weird bugs. (worst case)
  • If typing std:: is so tedious for you, try to import only truncated namespaces. ( e.g. individual identifiers like using std::string; or nested names like using namespace std::literals).
  • If importing nested namespaces still too awful for you, try to do it inside a restricted scope (e.g. a functions) and not in global scope.

Ensure validity of parameters

In your add(), substract(), multiply() and divide() function, you don't check the validity of given inputs. You could surely use assertions to ensure that the operation don't lead to integer overflow/underflow or division by zero.

You could use assertions to check preconditions, postconditions and invariants. It make your code more explicit and you avoid possibly broke you code when modifying.


Inputs / Outputs

  • When you read values from user input, consider it ill-formed, so check for validity and sanitize input. Never trust user, They all try to broke your program.
  • Using std::endl sending a '\n' and then flushes the output buffer. So std::endl is more expensive in performance. So, use '\n' '\n' and then, if you want to manually flush the stream, explicitly call std::flush.

Use your functions

If you don't know how to use your function, I think you really have to learn to basics. Their are a lot of resources on the net. Once you get basics, dive into the C++ FAQ and the C++ Core Guideline to get some good practices.

Also, it's Christmas soon, a good opportunity to ask for a good book about C++


Misc

  • Thank you so much for your feedback! – braisedbeefcake Dec 6 at 8:08
  • namespace std::string; is clearly an error, since std::string names a class, not a namespace. – Toby Speight Dec 6 at 9:55
  • 2
    return 0; is not necessary in main but I can't see any reason to recommend against doing it. What is you reason for suggesting this? – Jack Aidley Dec 6 at 10:27
  • @TobySpeight thx for pointing out this typo. – Calak Dec 6 at 11:48
  • 2
    @JackAidley By not using a return 0 is a convention that indicates that the application can never fail. Thus as a developer I don't need to look for the situations where the code will return an error. The convention says if your code can return an error (i.e. there are returns other than 0 elsewhere) then you should add the return 0 at the end. So if you see return 0 at the end you start looking for the error conditions. If you see no return 0 then you don't need to look for error situations the app is so simple it always just exits normally. – Martin York Dec 6 at 18:18

Default case!

In addition to the other excellent answers, you could add a default case to your switch statement! This is the case that is called when something other than the expected input is received.

In your example, it would end up looking something like this:

switch (user14)
{
    case 1: 
        Add(n1, n2);
        break;
    case 2:
        Subtract(n1, n2);
        break;
    case 3:
        Multiply(n1, n2);
        break;
    case 4:
        Divide(n1, n2);
        break;
    Default:
        cout << "Invalid operation\n";
        break;
}

In the above code, a 1, 2, 3, or 4 will perform the expected corresponding action, as in your original code. But in your original code, any other numerical input will just exit the switch statement and move on to the next thing, without telling the user that no action was taken. In the example above, it will instead tell the user why no action is being taken.

It's a useful way to let the user know that their input was invalid, but it can also be used to automatically take an action if none of the cases are matched. If that is confusing, you can think of default like the final "else" at the end of a series of "if/else if" statements.

On the other hand, if there's no need for user feedback and there's no "default" action that you want to happen normally, then there's no need to make a default case.

Another change I might suggest is for user14, I would personally use a char instead of an int. This will stop the program from crashing if the user inputs something other than an integer. But then, if you know that the input will only ever be an integer, then there's really no need to take that precaution!

Happy coding!

New contributor
MrSpudtastic is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.

Your Answer

braisedbeefcake is a new contributor. Be nice, and check out our Code of Conduct.

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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