My First Official Attempt

[sbwen fo gnik]

I decided to take up learning C++ (as I'm sure has been said an untold amount of times). For my first (but unoriginal) project, I decided to create a CLI Simple Arithmetic Calculator (1+1, 2-1, 3*1, 4/1). This is the logical description of my program:

* Display program title and available options to user
* Request input from user to select an option
- Return error if input is invalid and give another chance
* Perform arithmetic based on valid input
- Request input from user for calculation
- Return error if input is invalid and give another chance
- Return arithmetic calculation based on valid input
* Loop until user inputs exit option

Here is the extremely messy code I have come up with:

#include <iostream>
#include <string>
using namespace std;

//--- Global Variables
double a = 0;
double b = 0;
double c = 0;
int option = 0;
//--- End Global Variables

//--- Function Prototypes
void userOptions();
void optionLoop();
void validOptionCheck();
void validMathInputCheck();
double addition();
double subtraction();
double multiplication();
double division();
//--- End Function Prototypes

//--- Main
int main()
{
	// Program Title
	cout << "Simple Arithmetic Calculator \n" << endl;

	// Display User Options, Ask for Input, and Validate
	userOptions();
	validOptionCheck();
	optionLoop();

	return 0;
}
//--- End Main

//--- Function Definitions
void userOptions()
{
	// Ask For User Input
	cout << "What would you like to do?: \n\n";

	// User Options
	cout << "1. Add \n"
		<< "2. Subtract \n"
		<< "3. Multiply \n"
		<< "4. Divide \n"
		<< "5. Exit Program \n\n";

	// Get User Input
	cin >> option;
	cout << "\n";
}

void optionLoop()
{
	// While-Loop for Continuity Until Exit
	while(option != 5)
	{
		// Switch Case for Options
		switch(option)
		{
		case 1:
			if(option == 1)
			{
				cout << "What numbers would you like to add?\n";
				cout << "\n";
				cin >> a >> b;
				cout << "\n";
				validMathInputCheck();
				cout << a << " + " << b << " = " << addition() << "\n\n";

				// Show User Options and Get User Input
				userOptions();
				validOptionCheck();

			}
			break;
		case 2:
			if(option == 2)
			{
				cout << "What numbers would you like to subtract?\n";
				cout << "\n";
				cin >> a >> b;
				cout << "\n";
				validMathInputCheck();
				cout << a << " - " << b << " = " << subtraction() << "\n\n";

				// Show User Options and Get User Input
				userOptions();
			    validOptionCheck();

			}
			break;
		case 3:
			if(option == 3)
			{
				cout << "What numbers would you like to multiply?\n";
				cout << "\n";
				cin >> a >> b;
				cout << "\n";
				validMathInputCheck();
				cout << a << " * " << b << " = " << multiplication() << "\n\n";

				// Show User Options and Get User Input
				userOptions();
				validOptionCheck();

			}
			break;
		case 4:
			if(option == 4)
			{
				cout << "What numbers would you like to divide?\n";
				cout << "\n";
				cin >> a >> b;
				cout << "\n";
				validMathInputCheck();
				cout << a << " / " << b << " = " << division() << "\n\n";

				// Show User Options and Get User Input
				userOptions();
				validOptionCheck();

			}
			break;
		}
	}
}

void validOptionCheck()
{
	while(!option)
	{
		cout << "Invalid input. Please try again." << endl;
		cin.clear();
		cin.ignore(100,'\n');
		cin >> option;
		cout << "\n";
	}
}

void validMathInputCheck()
{
	while (!a || !b)
	{
		cout << "Error detected! Only numerical values are allowed." << endl;
		cin.clear();
		cin.ignore(100,'\n');
		cin >> a >> b;
		cout << "\n";
	}
}

double addition()
{
	c = (a + b);
	return c;
}

double subtraction()
{
	c = (a - b);
	return c;
}

double multiplication()
{
	c = (a * b);
	return c;
}

double division()
{
	c = (a / b);
	return c;
}
//--- End Function Definitions 

So far, the program works MOSTLY as expected. I used double for the variables for calculation so that users can return decimals as results. However, from the code I have now, there is an infinite loop somewhere that I just can't see. I've tried taking breaks and coming back, but I can't find it. The loop occurs when I enter invalid characters for calculation (ex. instead of entering two numbers, I do letters). On the first pass, I receive the correct error response and my code works as intended. However, when I try again, it loops infinitely (I don't know if what I'm saying is making sense, so forgive me).

Anyway, I'm opening myself up for some coding tips/insight. Also, is it normal to get so wrapped up in your code that it's hard to find mistakes? Does this change with experience?

[firedraco]

1) First, you shouldn't be using global variables for this. Pass those variables to your functions instead.

2) Inside your cases, right the case statement you immediately check the value option, which you just did with your case statement. Why did you do this?

3) The looping error you are getting due to the behavior of streams on invalid input. They go into an error state if an invalid value is entered. You will need to clear this after clearing out the invalid junk in the buffer.

4) Yes, and honestly, it doesn't to become easier for me really. You'll It is very useful to have someone else to talk to/go over your code with so you can find out errors.

[MrHutch]

Right, a few quick things. :-)

First, you get get around the input/loop problem by adding some extra checks to your input code.

std::cout << "Please enter a number between 1 and 5: ";

while( !( std::cin >> option ) || option < 1 || option > 5 )
{
   std::cout << "Incorrect number added, try again: ";
   std::cin.clear();
   std::cin.ignore( 256, '\n' );
}

This code will ensure that an integer is entered and that it is within the bounds you want. I can explain why this works exactly if you like, but it might be a bit confusing to you if you're just beginning. Just let me know if you want to know, though.

Second point to note is that the if statements inside of the case statements are redundant. The case statement is already evaluating the option variable, so there's little need to do it again in an if statement. You can remove those if statements and their braces and it would work fine.

Another thing that's worth noting is that it's always good to have a default case in your switch statement. This will be hit when anything other than a value you've written a case for has been entered.

switch( option )
{
case 1:
      { 
         std::cout << "Option 1\n";
         break;
      }
case 2:
      {
         std::cout << "Option 2\n";
         break;
      }
default:
      {
         std::cout << "Invalid option\n";
         break;
      }
}

One final observation is that the general flow could be tidied up a bit, but I suspect this will come with time. For your next project, it might be worth considering alternatives to global variables and look at passing arguments to functions. All in all, a good effort, though. :-)

sbwen fo gnik wrote:

Anyway, I'm opening myself up for some coding tips/insight. Also, is it normal to get so wrapped up in your code that it's hard to find mistakes? Does this change with experience?

Yeah, you still get wrapped up in your code and get stung by obvious errors. I spent a good half hour scratching my head at an error at work yesterday only to find that I'd missed the static keyword from a variable.

Edit: Wow, I was slow typing that. firedraco ninja'd me on every point!

[sbwen fo gnik]

Sorry for the late reply, but thank you for the responses. I decided to modify my code according to the tips I received in this thread. Here is the code:

#include <iostream>

//--- Function Prototypes
void optionLoop();
void add(double a, double b);
void subtract(double a, double b);
void multiply(double a, double b);
void divide(double a, double b);
//--- End Function Prototypes

//--- Main
int main()
{
    // Program Title
    std::cout << "Simple Arithmetic Calculator \n" << std::endl;

    // Displays User Options, Asks for Input, and Validates Input
    optionLoop();

    return 0;
}
//--- End Main

//--- Function Definitions
void optionLoop()
{
    // Store Selected Option
    int option;

    // Ask For User Input
    std::cout << "What would you like to do?: \n\n";

    // User Options
    std::cout << "1. Add \n"
         << "2. Subtract \n"
         << "3. Multiply \n"
         << "4. Divide \n"
         << "5. Exit Program \n\n";

    // Get User Input
    while (!( std::cin >> option) || option < 1 || option > 5)
    {
        std::cout << "Invalid option. Please try again: ";
        std::cin.clear();
        std::cin.ignore( 256, '\n' );
    }
    // New Line for Readability
    std::cout << "\n";

    if(option != 5)
    {
        // Switch Case for Options
        switch(option)
        {
        case 1:
            add(0, 0);
            break;
        case 2:
            subtract(0, 0);
            break;
        case 3:
            multiply(0, 0);
            break;
        case 4:
            divide(0, 0);
            break;
        default:
            std::cout << "Error detected!\n";
            break;
        }
    }
}
void add(double a, double b)
{
    std::cout << "What numbers would you like to add?\n\n";
    while(!(std::cin >> a >> b))
    {
        std::cout << "Only numerical values are allowed. Try again :\n";
        std::cin.clear();
        std::cin.ignore(100, '\n');
    }
    std::cout << "\n" << a << " + " << b << " = " << a + b << "\n\n";
    std::cin.clear();
    std::cin.ignore( 256, '\n' );
    optionLoop();
}
void subtract(double a, double b)
{
    std::cout << "What numbers would you like to subtract?\n\n";
    while(!(std::cin >> a >> b))
    {
        std::cout << "Only numerical values are allowed. Try again :\n";
        std::cin.clear();
        std::cin.ignore(100, '\n');
    }
    std::cout << "\n" << a << " - " << b << " = " << a - b << "\n\n";
    std::cin.clear();
    std::cin.ignore( 256, '\n' );
    optionLoop();
}
void multiply(double a, double b)
{
    std::cout << "What numbers would you like to multiply?\n\n";
    while(!(std::cin >> a >> b))
    {
        std::cout << "Only numerical values are allowed. Try again :\n";
        std::cin.clear();
        std::cin.ignore(100, '\n');
    }
    std::cout << "\n" << a << " * " << b << " = " << a * b << "\n\n";
    std::cin.clear();
    std::cin.ignore( 256, '\n' );
    optionLoop();
}
void divide(double a, double b)
{
    std::cout << "What numbers would you like to divide?\n\n";
    while(!(std::cin >> a >> b))
    {
        std::cout << "Only numerical values are allowed. Try again :\n";
        std::cin.clear();
        std::cin.ignore (100, '\n');
    }
    std::cout << "\n" << a << " / " << b << " = " << a / b << "\n\n";
    std::cin.clear();
    std::cin.ignore( 256, '\n' );
    optionLoop();
}
//--- End Function Definitions 

Everything works as expected EXCEPT error checking for input of the second arithmetic value. When I enter 2f or f2 for the first value, it returns an error like it should. However, error checking on the second value only partially works. For example, it returns an error when I enter f2. However, when I enter 2f no error is returned. The function continues to calculate the numbers and ignores any letters that follow. Why does this happen? Also, is my code better or worse?

[TheIdeasMan]

I would have a case for option 5, then there is no need for line 50.

There is also a lot of validation code in the functions - can you get the 2 numbers & validate them before entering the switch? You can send these numbers to the math functions, which is better than sending zero's. This will help in avoiding repetitive code.

Have fun with your coding - always good to see some learning happening 8+)