3
\$\begingroup\$

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

\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

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 :)

\$\endgroup\$
2
  • 1
    \$\begingroup\$ This is the updated code pastebin.com/0k78RqhM, i've been working on so, it utilises some of the improvements stated above if i'm not mistaken. \$\endgroup\$
    – Luka
    Dec 22, 2019 at 14:00
  • 1
    \$\begingroup\$ s/4/std::size(operations)/g \$\endgroup\$ Dec 22, 2019 at 21:06
6
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Also thank you, will look into fixing it, tips are always welcome! \$\endgroup\$
    – Luka
    Dec 22, 2019 at 10:19
4
\$\begingroup\$

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!

\$\endgroup\$
3
  • \$\begingroup\$ It helps thanks a lot i'll look into it! \$\endgroup\$
    – Luka
    Dec 22, 2019 at 10:18
  • \$\begingroup\$ Hello julian!, ive implemented the a and b variables change to my code, it looks much cleaner now. \$\endgroup\$
    – Luka
    Dec 22, 2019 at 11:10
  • \$\begingroup\$ Great ! @Luka . \$\endgroup\$
    – julianff
    Dec 22, 2019 at 11:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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