Critique and Optimize my coding?

[Elarionus]

For this project, we were supposed to set up a calculator program, where you could enter a number and either use addition, subtraction, multiplication, division, or summation. A large portion of the class decided to make it with loops and other things, but I decided to do it with functions. I had endless amounts of troubleshooting, trying to figure out why certain functions weren't working and why certain functions returned 0 when it should move to another function...I eventually ended up with this, but I want to know, what ways could this coding be better, based on the parameters I was given?

  /* Ethan Dale
 * Section 02
 * 02/06/18
 * Lab 04
 */

#include <iostream>
using namespace std;


int menu();
int selection();
int addition();
int subtraction();
int multiplication();
int division();
int start();
int sumofintegers();


int main()
{
	int a = start();
	while (a == 1)
	{
		menu();
		selection();
		a = start();
	}

	return 0;
}

int start()
{
	int start;
	cout << "\nThis is a calculator program. Press 1 to begin or press 0 to exit.\n";
	cin >> start;
	if (start == 1)

		return 1;

	if (start == 0)

		return 0;
}

int menu()
{
	cout << "Press the number corresponding to the function you need.\n";
	cout << "1 = Addition\n2 = Subtraction\n3 = Multiplication\n4 = Division\n5 = Sum of Integers\n\n";

	return 0;
}

int selection()
{
	int n;
	cin >> n;

	while (n < 1 || n > 5)
	{
		cout << "Invalid selection. Please select either 1, 2, 3, 4, or 5.\n";
		cin >> n;
	}

	if (n == 1)

	{
		addition();
	}

	if (n == 2)
	{
		subtraction();
	}

	if (n == 3)
	{
		multiplication();
	}

	if (n == 4)
	{
		division();
	}

	if (n == 5)
	{
		sumofintegers();
	}

	return 0;

}



int addition()
{
	double a = 0.0;
	double b = 0.0;
	double sum = 0.0;
	cout << "Enter a number.\n";
	cin >> a;
	cout << "Enter another number.\n";
	cin >> b;

	sum = (a + b);

	cout << "The sum is:\n";
	cout << sum;
	cout << "\n";

	return 0;
}


int subtraction()
{
	double a = 0.0;
	double b = 0.0;
	double difference = 0.0;
	int total = 0;
	cout << "Enter a number.\n";
	cin >> a;
	cout << "Enter another number.\n";
	cin >> b;

	difference = (a - b);

	cout << "The difference is:\n";
	cout << difference;
	cout << "\n";

	return 0;
}


int multiplication()
{
	double a = 0.0;
	double b = 0.0;
	double product = 0.0;
	cout << "Enter a number.\n";
	cin >> a;
	cout << "Enter another number.\n";
	cin >> b;

	product = (a * b);

	cout << "The product is:\n";
	cout << product;
	cout << "\n";

	return 0;
}


int division()
{
	double a = 0.0;
	double b = 0.0;
	double quotient = 0.0;
	cout << "Enter the dividend.\n";
	cin >> a;
	cout << "Enter the divisor.\n";
	cin >> b;

	quotient = (a / b);

	cout << "The quotient is:\n";
	cout << quotient;
	cout << "\n";

	return 0;

}



int sumofintegers()
{
	double a = 0.0;
	double b = 0.0;
	int total = 0;

	cout << "Enter the starting number.\n";
	cin >> a;
	cout << "Enter the ending number.\n";
	cin >> b;

	int x = a;
	int y = b;

	for (int z = x; z <= y; z++)
	{
		total = total + z;
	}

	cout << "The sum of all the intergers between your two numbers is:\n";
	cout << total << "\n";

	return 0;
}

[dhayden]

Let me start by saying that this is nice clean code for a beginner. The fact that you're reaching out for opinions shows a great willingness to learn. Go you!!

The loop in main could be simply:

while (start()) {
    menu();
    selection();
};

The name start() is misleading since it's run each time through the loop. Maybe keepGoing() or something like that. Whatever you call it, it would be better to return bool instead of int. This would better reflect the fact that it returns a yes/no status.

Line 39-45 could be replace with:
return (start == 1);

Any time you find yourself writing the same code several times, you should see if you can factor it out into a separate function. For example, addition(), subtraction(), multiplication() and addition() all have the same code to prompt for two numbers:

// Prompt for two numbers and put them in "a" and "b"
void getTwo(double &a, double &b)
{
    cout << "Enter a number.\n";
    cin >> a;
    cout << "Enter another number.\n";
    cin >> b;
}

Note that a and b are passed by reference.

Then addition() becomes simply:

int
addition()
{
    double a, b;
    double sum = 0.0;

    getTwo(a,b);
    sum = (a + b);

    cout << "The sum is:\n";
    cout << sum;
    cout << "\n";

    return 0;
}

Caution: I haven't initialized a and b because I know that any value will be overwritten in getTwo(). As a beginner, you might want to always initialize variables to avoid the common bug of using an uninitialized one.

In selection(), you should use a switch() statement instead of repeated if's. Not only is this potentially faster, but it better reflects that fact that you're executing different code depending on the value of n:

    switch(n) {
    case 1:
        addition();
        break;
    case 2:
        subtraction();
        break;
    case 3:
        multiplication();
        break;
    case 4:
        division();
        break;
    case 5:
        sumofintegers();
        break;
    }

In division(), you should probably check for division by zero and print an appropriate error message if it's attempted.

Line 198 can be simplified with the += operator:
total += z;

[jonnin]

It IS very good!

This builds on what was said above, digging into the why a little more.

consider making the functions like division have results and parameters.
eg

double division(double a, double b)
{
if(b)
return a/b;
//else handle error somehow, etc
}

this allows you to use the function without any prompts (say you needed to divide one list of 1000 numbers by a second list of 1000 numbers) or with custom prompts (using the ideas above, how you GET a&b no longer matters, could be user input, could be a file, etc). It becomes more reusable because it only does one thing and can be called easily from a variety of different code blocks, whereas the way you have it is very specific to your current program. Granted, its a bit of a silly example but hopefully you can see that by doing just 1 thing in the function you can now have flexibility; you can call it in a loop over many numbers with no user I/O at all, you can do what you did by putting a getnumbers and a printresults before and after it, you can even string up a complex equation like
finalresult = multiplication(division(a,b), subtraction(c,d));

If you do things like this and turn it in for a grade, be prepared to explain why as your professor may test you to see if you are cheating or actually just ahead of the class. Being ahead of the class is a good thing, as long as you can show that you did the work and understand the ideas.

[Elarionus]

Thank you guys for the replies! I agree with you jonnin, I'd much prefer to learn the practicality of why the code works the way it does, rather than copy and paste from online. That'll get me nowhere in the world. ;P

I'll try making many of the adjustments you guys suggested. We're just now getting into functions in our lecture class, so I'm still learning how they work with each other. My professor has a habit of jumping into very advanced topics without explaining the basic things, so I'm catching up using both the "lab" sessions and the learnc++ site. I'll make the changes you suggested to the code, and if I run into any issues, I'll either post them here, or make a new thread, if it's different enough.

Again, thank you for the advice!