First calculator in c++, tips for optimisation

Question

So this is my first attempt at making a calculator in c++, and i'm looking for experienced people to pinpoint what i could do better to make my calculator run better! *please note that i had to translate some parts of the code to english from my native language, and had to change bunch of names so if i forgot something somewhere my bad :P

#include <cmath>
#include <iostream>
#include <stdio.h>

using namespace std;

int prompt(const char* name) 
{
printf("%s", name);
int value;
scanf("%d", &value);
return value;
}

int main()
{   
    int mod;
    cout << "U kojem modu kalkulatora zelis biti? " << mod << endl;
    cout << "Mod 1 je za zbrajanje" << endl;
    cout << "Mod 2 je za oduzimanje" << endl;
    cout << "Mod 3 je za mnozenje" << endl;
    cout << "Mod 4 je za dijeljenje" << endl;
    cin >> mod;

    switch (mod)
    {
        case 1:
            cout << "Odabrali ste zbrajanje!" << endl; 
            int a, b, zbroj;
            a = prompt("a: ");
            b = prompt("b: ");
            zbroj = a + b;
            printf("%d + %d = %d", a, b, zbroj);
            break;
        case 2: 
            cout << "Odabrali ste oduzimanje!" << endl; 
            int razlika;
            a = prompt("a: ");
            b = prompt("b: ");
            razlika = a - b;
            printf("%d - %d = %d", a, b, razlika);
            break;
        case 3: 
            cout << "Odabrali ste mnozenje!" << endl; 
            int umnozak;
            a = prompt("a: ");
            b = prompt("b: ");
            umnozak = a * b;
            printf("%d * %d = %d", a, b, umnozak);
            break;
        case 4: 
            cout << "Odabrali ste dijeljenje!" << endl;
            int kolicnik;
            a = prompt("a: ");
            b = prompt("b: ");
            kolicnik = a / b;
            printf("%d / %d = %d", a, b, kolicnik);
            break;

        default:
            cout << "Nemamo tu opciju trenutno, prcekajte za nadogradnju programa, ili se javite developeru!" << endl;
    }
    return 0;
}

I'm a newbie :P

Answer 1

Just adding to the tips already given in the other answers:

Don't mix iostream and stdio functions

Stick to one. Since you are writing C++, go for iostream functions. Note that there are libraries, like fmtlib that give you printf-like formatting that works with std::string and std::ostreams, which allows you to write something like:

#include <fmt/ostream.h>
...
fmt::print(std::cout, "{} + {} = {}\n", a, b, zbroj);

Avoid repetition

Each of the four cases prompt for two input values before printing the result. There is a lot of code duplication there. That will make it hard whenever you want to change something in the behavior of your code. So whenever you spot repetition, try to avoid it by creating a function or a class that generalizes the thing you are trying to achieve.

For example, you can make the printing of the first line and reading in the two variables into a function:

void get_values(const char *text, int &a, int &b) {
    std::cout << text << '\n';
    a = prompt("a: ");
    b = prompt("b: ");
} 

And then use that in your switch-statement as follows:

int a, b;

switch (mod) {
case 1:
    get_values("Odabrali ste zbrajanje!", a, b);
    std::cout << a << " + " << b << " = " << (a + b) << 'n';
    break;
case 2:
    get_values("Odabrali ste oduzimnje!", a, b);
    std::cout << a << " - " << b << " = " << (a - b) << 'n';
    break;
case ...
}

You can go even further than that, although if you are not comfortable with this level of C++, just use the simple approach above:

#include <functional>
...
void calculate(const char *name, char symbol, std::function<int(int, int)> operation) {
    std::cout << "Odabrali ste " << name << "!\n";
    int a = prompt("a: ");
    int b = prompt("b: ");
    std::cout << a << symbol << b << " = " << operation(a, b) << 'n';
}

And use that in the switch-statement as follows:

switch (mod) {
case 1:
    calculate("zbrajanje", '+', [](int a, int b){return a + b;});
    break;
case 2:
    calculate("oduzimanje", '-', [](int a, int b){return a - b;});
    break;
case ...
}

Instead of writing these lambda functions by hand, you could also use std::plus, std::minus et cetera from <functional>.

The data-driven approach

Currently you are using control flow (the switch-statement) to decide what to do given the initial input. There is nothing wrong with that, but there is another way that also prevents repetition, and that is by storing all the information about the operations in a table, and consult this table instead of using the switch-statement. Here is what it would look like:

#include <functional>

struct operation {
    const char *name;
    char symbol;
    std::function<int(int, int)> function;

    void calculate();
};

static const operation operations[] = {
    {"zbrajanje",  '+', std::plus<int>},
    {"oduzimanje", '-', std::minus<int>},
    ...
};

operation::calculate() {
    std::cout << "Odabrali ste " << name << "!\n";
    int a = prompt("a: ");
    int b = prompt("b: ");
    std::cout << a << symbol << b << " = " << function(a, b) << 'n';}
}

And instead of the switch-statement, write:

if (mod >= 1 && mod <= 4) {
    operations[mod - 1].calculate();
} else {
    std::cout << "Nemamo tu opciju trenutno, prcekajte za nadogradnju programa, ili se javite developeru!\n";
}

Now you can also generate the help text you print in the beginning in an automated way:

std::cout << "U kojem modu kalkulatora zelis biti?\n";
for (int i = 1; i <= std::size(operation); ++i) {
    std::cout << "Mod " << i << "je za " << operations[i - 1].name << "\n";
}

Again, if you are a beginner, this might seem a bit much. Don't worry about it, try to get comfortable with the basics first. Just remember that the language provides you with a lot of tools to make your life easier :)

Answer 2

You should avoid using namespace std.

The code in the prompt function is not indented.

You should avoid using endl unless absolutely necessary (use a \n instead). endl will flush the output buffer which can be a big performance hit.

You don't have a break statement for the default case of your switch statement. While not required here, it can lead to problems in the future if additional cases are added after the default and the break is not added.

Declaring uninitialized variables within a switch statement is legal, but can lead to confusion. Usually when variables are declared under a case, the code of the case is enclosed within { curly brackets }. For example, the a and b variables, which are used in most of the cases, could be declared before the switch.

Answer 3

a couple of things -

1. a and b variables which are declared in the first case statement are undeclared in the next case statements. Better to declare those outside the switch case block.

2. Divide by 0. If user enters b as 0 in the 4th case, it will cause undefined behaviour - i.e. literally anything could happen.

3. The prompt function is unindented. Its better to have indented code for readability, although it doesn't have much effect on performance.

Hope these help!