Htykuut

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/nn";



        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!

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.

• 
  How to use assertions in C (for C, but still applicable here)


• 
  How and When to Use C's assert() Macro (same, also valid)


• 
  Why should I use asserts?

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

• When it's possible, try to avoid using goto.


• 
  Don't return 0 from main().


• You should also fix indentation.

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 operationn";

        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!