# Thread: Is my code quality clean readable?

1. ## Is my code quality clean readable?

Purpose of code:-
I did this mainly for sign specific calculation(+,-,/,*)using 2 numbers input ny user, then finding the greater no input by user and lastly doing square rt of that no. Is this code good enough as a clean code; if not, what changes can I implement in it?

Code:
```#include <iostream>
#include <cmath>

int main() {

int num1, num2, sumoftwonumbers, subtoftwonumbers, DivOftwonumbers, MultiplyingTwoNumbers, OpSign;
// Above Line declares all the variables to be used in this calculator program

std::cout << "Type first number = " << std::endl;
std::cin >> num1;
std::cout << "Type second number = " << std::endl;
std::cin >> num2;
std::cout << std::endl; //breaks line, std:: refers to standard library

sumoftwonumbers = num1 + num2;
subtoftwonumbers = num1 - num2;
DivOftwonumbers = num1 / num2;
MultiplyingTwoNumbers = num1 * num2;

std::cout << "Type your Operation Sign: 1 for (+), 2 for (-), 3 for (/), 4 for (*),---- ";
std::cin >> OpSign;

switch(OpSign){
case 1:
std::cout << "The sum of first and second number is : " << sumoftwonumbers << std::endl;
break;
case 2:
std::cout << "The subtraction of first and second number is : " << subtoftwonumbers << std::endl;
break;
case 3:
std::cout << "The division of first and second number is : " << DivOftwonumbers << std::endl;
break;
case 4:
std::cout << "The multiplication of first and second number is : " << MultiplyingTwoNumbers << std::endl;
break;
default:
std::cout << "Invalid Operation! ";
}
// function of calculator ends here

//using if-else conditionals to display larger number
if(num1 > num2){
std::cout << "Num1 is greater"; // If Num1 is greater displays this
}else{
std::cout << "Num2 is greater"; // If Num2 is greater displays this
}

/* These blocks of codes help in addition of two numbers and
finding out the greater number from them */

std::cout << std::endl; // EMPTY LINE.

//finding square root of greater number in code below
if(num2 > num1){
std::cout << std::endl << std::endl << "Square root of greater number(num2) is = " << std::sqrt(num2);
}
else{
std::cout << "Square root of greater number(num1) is = " << std::sqrt(num1);
}

std::cout << std::endl << std::endl << "Have a good day";//This function prints simply any text between " and "
return 0;
}```

2. Your comments document the language, not what your program does.
Although having said that, some tutors seem to like that kind of stuff.

The only semi-useful comment would be the one "/* These blocks of codes..." but that appears after the code it relates to.
Conventionally, you comment first (if you have anything useful to say), then you code.
Useful means anything that is not obvious from reading the code.

Your main() is way too long.
As a rough rule of thumb, if you can't see the opening and closing braces of a function on screen at the same time, then it's time to start splitting things up.
Long switch/case statements are OK so long as each case is simple in it's own right.

There are a couple of coding mistakes that should be fixed.
1. There is no protection against divide by zero.
2. There is no protection against square root of a negative number.

Whilst it is convention that default: is last in a switch/case, it isn't mandatory.
So always add a break;

Code:
```#include <iostream>
#include <cmath>

void inputTwoNumbers(int &num1, int &num2) {
std::cout << "Type first number = " << std::endl;
std::cin >> num1;
std::cout << "Type second number = " << std::endl;
std::cin >> num2;
}

void fourFunctionCalculator(int num1, int num2) {
int sumoftwonumbers = num1 + num2;
int subtoftwonumbers = num1 - num2;
int DivOftwonumbers = num1 / num2;
int MultiplyingTwoNumbers = num1 * num2;
std::cout << "Type your Operation Sign: 1 for (+), 2 for (-), 3 for (/), 4 for (*),---- ";
int OpSign;
std::cin >> OpSign;
switch(OpSign){
case 1:
std::cout << "The sum of first and second number is : " << sumoftwonumbers << std::endl;
break;
case 2:
std::cout << "The subtraction of first and second number is : " << subtoftwonumbers << std::endl;
break;
case 3:
std::cout << "The division of first and second number is : " << DivOftwonumbers << std::endl;
break;
case 4:
std::cout << "The multiplication of first and second number is : " << MultiplyingTwoNumbers << std::endl;
break;
default:
std::cout << "Invalid Operation! ";
break;
}
}

void largestWithRoot(int num1, int num2) {
if(num1 > num2) {
std::cout << "Num1 is greater" << std::endl;
std::cout << "Square root of greater number(num1) is = " << std::sqrt(num1) << std::endl;
}else{
std::cout << "Num2 is greater" << std::endl;
std::cout << "Square root of greater number(num2) is = " << std::sqrt(num2) << std::endl;
}
}

int main() {
int num1, num2;
inputTwoNumbers(num1,num2);
fourFunctionCalculator(num1,num2);
largestWithRoot(num1,num2);
}```

3. Besides what Salem said, I'd like to add two things:
1. Identifiers. On one side you have those that name their variables 'a' and 'foo'. This is bad. On the other, you have 'thisVariableHoldsTheSumOfTwoNumbers' this is also bad, or worse, I'd argue. Yours aren't terrible, but still overly descriptive. With a bit of thought, you can improve things:
Code:
```sumOfTwoNumbers -> sum
subtOfTwoNumbers -> difference
divOfTwoNumbers -> quotient
multiplyingTwoNumbers -> product```
2. Instead of using numbers that translate into operations, It'd be much clearer to use the appropriate character. e.g:
Code:
```    std::cout << "Type your Operation: '+', '-', '\\' or '*'),---- ";
int OpSign;
std::cin >> OpSign;
switch(OpSign){
case '+':
std::cout << "The sum of first and second number is : " << sum << std::endl;
break;
case '-':
std::cout << "The subtraction of first and second number is : " << difference << std::endl;
break;
case '\\':
std::cout << "The division of first and second number is : " << quotient << std::endl;
break;
case '*':
std::cout << "The multiplication of first and second number is : " << product << std::endl;
break;
default:
std::cout << "Invalid Operation! ";
break;
}```

Popular pages Recent additions