C++ cleaner optimised code, Calculator

Question

This code currently works i wonder if i could make any optimisations or anything to make the code appear cleaner using functions or whatnot, suggestions are welcome!

#include <cmath>
#include <cstdio>
int prompt(const char* name) 
{
printf("%s", name);
int value;
scanf("%d", &value);
return value;
}
int main()
{ 
 int mod, a, b, zbroj, razlika, umnozak, kolicnik;
 printf("Mod 1 je za zbrajanje \n");
 printf("Mod 2 je za oduzimanje \n");
 printf("Mod 3 je za mnozenje \n");
 printf("Mod 4 je za dijeljenje \n");
 printf("U kojem modu zelis biti: \n");
 std::scanf("%d", &mod);
 switch (mod)
 {
 case 1:
 printf("Odabrali ste zbrajanje! \n");
 a = prompt("a: ");
 b = prompt("b: ");
 zbroj = a + b;
 printf("%d + %d = %d", a, b, zbroj);
 break;
 case 2: 
 printf("Odabrali ste oduzimanje! \n");
 a = prompt("a: ");
 b = prompt("b: ");
 razlika = a - b;
 printf("%d - %d = %d", a, b, razlika);
 break;
 case 3: 
 printf("Odabrali ste mnozennje! \n");
 a = prompt("a: ");
 b = prompt("b: ");
 if(a == 0 or b==0){
 printf("Mnozite s nulom, a kad se mnozi s nulom rezultat je uvijek 0: \n");
 umnozak = 0;
 }
 else
 {
 umnozak = a * b;
 }
 printf("%d * %d = %d", a, b, umnozak);
 break;
 case 4: 
 printf("Odabrali ste dijeljenje! \n");
 a = prompt("a: ");
 b = prompt("b: ");
 if(a == 0 or b==0){
 printf("Dijelite s nulom, a kad se dijeli s nulom rezultat je uvijek 0: \n");
 kolicnik = 0;
 }
 else
 {
 kolicnik = a / b;
 }
 printf("%d / %d = %d", a, b, kolicnik);
 break;
 default:
 printf("Nemamo tu opciju trenutno, prcekajte za nadogradnju programa, ili se javite developeru! \n");
 }
 return 0;
}

Answer 1

Here are some things that may help you improve your code.

Check return values for errors

The call scanf can fail. You must check the return values to make sure it hasn't or your program may crash (or worse) when given malformed input or due to low system resources. Rigorous error handling is the difference between mostly working versus bug-free software. You should strive for the latter.

Decompose your program into functions

Almost all of the logic here is in main in one rather long and dense chunk of code. It would be better to decompose this into separate functions.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Know when to use it and when not to (as when writing include headers). In this particular case, I happen to think it's perfectly appropriate because it's a single short program that and not a header. Some people seem to think it should never be used under any circumstance, but my view is that it can be used as long as it is done responsibly and with full knowledge of the consequences.

Use <cstdio> instead of <stdio.h>

The difference between the two forms is that the former defines things within the std:: namespace versus into the global namespace. Language lawyers have lots of fun with this, but for daily use I'd recommend using <cstdio>. See this SO question for details.

Prefer iostream to old-style printf

The iostream library is the more modern C++ way to do I/O. Prefer it over printf and scanf. So instead of this:

printf("%d + %d = %d", a, b, zbroj);

Write this:

std::cout << a << " + " << b << " = " << zbroj << '\n';;

See SL.io.3 for details.

Initialize variables before use

The mod variable is printed before it is set to any value. That is not likely to be useful. Better is to always make sure that all variables are initialized before using them.

Use constant string concatenation

This code currently includes these lines:

cout << "U kojem modu kalkulatora zelis biti? \n" << mod;
cout << "Mod 1 je za zbrajanje \n";
cout << "Mod 2 je za oduzimanje \n";
cout << "Mod 3 je za mnozenje \n";
cout << "Mod 4 je za dijeljenje \n";

I would suggest writing it instead like this:

std::cout << 
 "U kojem modu kalkulatora zelis biti? \n" 
 "Mod 1 je za zbrajanje \n"
 "Mod 2 je za oduzimanje \n"
 "Mod 3 je za mnozenje \n"
 "Mod 4 je za dijeljenje \n";

Since the next line happens to be scanf, I'd suggest instead using your prompt function.

Consider using more idiomatic C++

While your use of and and not is not techncially wrong, you should be aware that many experienced C++ programmers will be unfamiliar with these operator alternatives, and so if others read your code, it might be an impediment to their understanding of the code.

Eliminate redundant variables

I don't speak Croatian, but the variable names are good in that they accurately describe what the variables mean in the context of the program. However, I would suggest that instead of zbroj, razlika, umnozak, kolicnik you could use a single variable rezultat.

Don't Repeat Yourself (DRY)

The mathematical operations in the case statement all include very similar repeated code. Instead of repeating code, it's generally better to make common code into a function. The only thing that varies is the prompt, the mathematical operation and the way the result is printed. An advanced technique would be to gther them all into a data structure. If you're just beginning, don't worry if you can't understand this yet. It uses lambda expressions and C++17 std::string_view among other things. I am putting it here to show how powerful C++ can be:

#include <cmath>
#include <iostream>
#include <string_view>
#include <array>
int prompt(const char* name) 
{
 std::cout << name;
 int value;
 std::cin >> value;
 return value;
}
int main()
{ 
 struct MathOp {
 std::string_view opname;
 int (*operate)(int a, int b);
 std::string_view opsymbol;
 void operator()() const {
 std::cout << "Odabrali ste " << opname << "!\n";
 int a = prompt("a: ");
 int b = prompt("b: ");
 int rezultat = operate(a, b);
 std::cout << a << opsymbol << b << " = " << rezultat << '\n';
 }
 };
 constexpr std::array<MathOp, 4> op{{
 { "zbrajanje", [](int a, int b){ return a+b; }, " + " },
 { "oduzimanje", [](int a, int b){ return a-b; }, " - " },
 { "mnozenje", [](int a, int b){ return a*b; }, " * " },
 { "dijeljenje", [](int a, int b){ 
 if (b == 0) { 
 std::cout << "dijeljenje s nulom \n";
 return 0;
 } 
 return a/b;
 }, " / " },
 }};
 std::cout << "U kojem modu kalkulatora zelis biti? \n";
 for (std::size_t i{0}; i < op.size(); ++i) {
 std::cout << "Mod " << i+1 << " je za " << op[i].opname << "\n";
 }
 unsigned mod;
 std::cin >> mod;
 if (mod > 0 && mod <= op.size()) {
 op[mod-1]();
 } else {
 std::cout << "Nemamo tu opciju trenutno, prcekajte za nadogradnju programa, ili se javite developeru! \n";
 }
}

Now if we want to add exponentiation, for example, all we need to do is add a single line to the op array:

{ "eksponenciju", [](int a, int b){ return static_cast<int>(std::pow(a,b)); }, " ** " },

• Luka: Thanks for taking your time to explain it to me, and yes its helpful, here's my updated code pastebin.com/nX65McD0, and oh god that's a lot of code you've got there which i cannot understand i'll do my best to understand it and apply it to mine over the period of me learning the cpp!
• Edward: It takes time to become expert in C++, but the reward is great. Keep at it, and I'm sure you'll get better quickly!
• pacmaninbw: Actually you have to modify the array size as well, it might be better to use std::vector.
• Edward: Yes, you have to modify the array size, but if you use a std::vector it can't be constexpr.
• Deduplicator: @pacmaninbw Or a native auto-sized array. Which has no disadvantages as there is no need to copy or resize it.

Answer 2

1) Be consistent when placing braces. The following is not consisent:

if(a == 0 or b==0){
 printf("Mnozite s nulom, a kad se mnozi s nulom rezultat je uvijek 0: \n");
 umnozak = 0;
}
else
{
 umnozak = a * b;
}

Either write it like this:

if (a == 0 or b==0) {
 printf("Mnozite s nulom, a kad se mnozi s nulom rezultat je uvijek 0: \n");
 umnozak = 0;
} else {
 umnozak = a * b;
}

or as

if (a == 0 or b==0)
{
 printf("Mnozite s nulom, a kad se mnozi s nulom rezultat je uvijek 0: \n");
 umnozak = 0;
}
else
{
 umnozak = a * b;
}

You can choose whatever style you like best, but you should stick to it.

2) Don't declare variables before they are used. Delay declaring variables until you actually need them. So instead of

int umnozak;
... some code here ...
umnozak = a * b;

You write

int umnozak = a * b;

This causes scoping issues in the clauses of the switch statement so they now have to be surrounded with braces.

3) Handle the division by zero better. It is not true that, say, 4 / 0 = 0, the result is undefined.

4) The variables zbroj and razlika are only ever used once. Most of the time such variables can and should be eliminated, which is also the case here.

The final code, formatted with a compact brace style, would look like this:

#include <stdio.h>
int prompt(const char* name) {
 printf("%s", name);
 int value;
 scanf("%d", &value);
 return value;
}
int main() {
 printf("Mod 1 je za zbrajanje\n");
 printf("Mod 2 je za oduzimanje\n");
 printf("Mod 3 je za mnozenje\n");
 printf("Mod 4 je za dijeljenje\n");
 printf("U kojem modu zelis biti:\n");
 int mod;
 scanf("%d", &mod);
 switch (mod)
 {
 case 1: {
 printf("Odabrali ste zbrajanje!\n");
 int a = prompt("a: ");
 int b = prompt("b: ");
 printf("%d + %d = %d\n", a, b, a + b);
 break;
 }
 case 2: {
 printf("Odabrali ste oduzimanje!\n");
 int a = prompt("a: ");
 int b = prompt("b: ");
 printf("%d - %d = %d\n", a, b, a - b);
 break;
 }
 case 3: {
 printf("Odabrali ste mnozennje!\n");
 int a = prompt("a: ");
 int b = prompt("b: ");
 printf("%d * %d = %d\n", a, b, a * b);
 break;
 }
 case 4: {
 printf("Odabrali ste dijeljenje! \n");
 int a = prompt("a: ");
 int b = prompt("b: ");
 if (b == 0) {
 printf("%d / %d = undefined (divison by zero)\n", a, b);
 } else {
 printf("%d / %d = %d\n", a, b, a / b);
 }
 break;
 }
 default:
 printf("Nemamo tu opciju trenutno, prcekajte za nadogradnju programa, ili se javite developeru! \n");
 }
 return 0;
}

• Luka: Oh wow, thanks! i just prefer the way i write it with "{ }", because its more readable for me and i'm a beginner still soo..
• Luka: also doesn't division by 0 always result with a 0, at least i learnt so by math :P
• Deduplicator: @Luka Curious. Generally in maths division by zero is undefined. Also, in C and C++, dividing the smallest possible value by -1 is undefined for integral numbers, as the result is not representable.
• David K: @Luka I'm curious where this unusual kind of division is taught. I also wonder why you print special messages when either of the operands of the multiplication is zero; a * b will happily return the correct answer in that case without any special help. Also, a / b will return the correct answer, 0, if a is zero, as long as b is not also zero. If I were doing this I would have checked only for b == 0 in the division code.