4
\$\begingroup\$

This code works as it is supposed to:

#include<iostream>
#include<cmath>
#include<cstdlib>

using namespace std;

int main()
{
    char cAgain, type;
    int x, y=0;
    double addition(double num1, double num2), subtraction(double num1, double num2), division(double num1, double num2), 
        multiplication(double num1, double num2), exponential(double num1, double num2), num1, num2, total;
    do
    {
        cout << "How many operations would you like to do?" << endl;
        cin >> x;
        if (x <= 0)
        {
            cout << "Error: You must have 1 or more operations." << endl;
        }
        else
        {
            while (x != 0, x--)
            {
                y++;
                cout << "Enter in your "<<y<< " operation. (First Number (+,-,*,/,^) Second Number)" << endl;
                cin >> num1 >> type >> num2;
                switch (type)
                {
                case '+':
                    total = addition(num1, num2);
                    cout << num1 << " + " << num2 << "= " << total<< endl;
                    break;
                case'-':
                    total = subtraction(num1, num2);
                    cout << num1 << " - " << num2 << "= " << total<< endl;
                    break;
                case'/':
                    total = division(num1, num2);
                    cout << num1 << " / " << num2 << "= " << total<< endl;
                    break;
                case'*':
                    total = multiplication(num1, num2);
                    cout << num1 << " * " << num2 << "= " << total << endl;
                    break;
                case'^':
                    total = exponential(num1, num2);
                    cout << num1 << " ^ " << num2 << "= " << total << endl;
                    break;
                }
            }
        }
        cout << "Would you like to run the program again.(Y/N)" << endl;
            cin >> cAgain;
    } while (cAgain == 'Y' || cAgain == 'y');
return 0;
}

double addition(double num1, double num2)
{
    double total;
    total = num1 + num2;
    return (total);
}
double subtraction(double num1, double num2)
{
    double total;
    total = num1 - num2;
    return (total);
}
double division(double num1, double num2)
{
    double total;
    total = num1 / num2;
    return (total);
}
double multiplication(double num1, double num2)
{
    double total;
    total = num1 * num2;
    return (total);
}
double exponential(double num1, double num2)
{
    double total;
    total = pow(num1,num2);
    return (total);
}

It starts by asking for the number of operations to perform. You enter a number, such as 3. Then it asks you to enter in your operation, such as 5+5, which is 10. Then it asks for a second, such as 10^2, which is 100. Then it asks for a third, such as 100-10, which is 90. Then it asks if you'd like to start over.

\$\endgroup\$
  • 4
    \$\begingroup\$ This is not the right place for such questions. Here, it is expected that you have working code but want to improve the quality of your code. \$\endgroup\$ – R Sahu Dec 24 '14 at 3:51
  • \$\begingroup\$ This code does work, wrote it without errors, I'm trying to improve the quality of the code by making it more user friendly. As it works now it works like your old school basic calculator it is hitting the = sign after every operation. I'm looking for a way to make it more like a modern scientific calculator. \$\endgroup\$ – Styx Dec 24 '14 at 4:17
  • \$\begingroup\$ Where's the modulo? :( \$\endgroup\$ – Yodism Dec 24 '14 at 4:49
  • 1
    \$\begingroup\$ You should look up the Command Pattern. Then use a std::map<char, Command> \$\endgroup\$ – Martin York Dec 24 '14 at 4:57
  • 1
    \$\begingroup\$ I have removed the off-topic part of the question. For inspiration, see questions tagged with math-expression-eval. \$\endgroup\$ – 200_success Dec 24 '14 at 5:09
9
\$\begingroup\$

There's a few issues here with c++ basics:

using namespace std

using namespace std;

This is usually a code smell, generally speaking the only time you would really want to do this is when you are making some small throwaway program for testing a concept or making an example (or similar). In that case the reduction in typing time actually has a positive ROI. But you don't want to do it in any production code because this pollutes the global namespace which is something you should try to avoid, any benefit from time saved typing is immediately wiped out the first time your program breaks because a name conflict. See this StackOverflow question for more info on that.

If the main purpose for doing this is to cut down typing std:: then you can selectively bring in just the names you need by doing:

using std::cout;
using std::cin;

an so on. This cuts down on typing without the downside of polluting the global namespace.

Function declarations

Currently you have this line:

double addition(double num1, double num2), subtraction(double num1, double num2), division(double num1, double num2), 
    multiplication(double num1, double num2), exponential(double num1, double num2), num1, num2, total;

First of all, this line is way too many characters wide to be readable. But more importantly you are declaring some functions and some variables on the same line. This is very bad for readability. Some people only have one declaration per line, but at the very least don't have more than one type of thing being declared on the same line.

The idiomatic c++ way to do function declarations is at the top of the file or in header files. Instead put the function declarations above main one per line:

double addition(double num1, double num2);
double subtraction(double num1, double num2);
double division(double num1, double num2);
double multiplication(double num1, double num2);
double exponential(double num1, double num2);

This way it's much clearer to the readers of your code which functions you are forward declaring.

Unnecessary variables

In each of the functions that computes the value of an expression you have the same structure:

double addition(double num1, double num2)
{
    double total;
    total = num1 + num2;
    return (total);
}

This can be simplified by just doing:

double addition(double num1, double num2)
{
    return num1 + num2;
}

The reduction in the number of lines of code is a win for readability.

Improving the features of the program

In order to compute the expressions such as 5+5^2-10 with correct operator precedence you will need to implement a parser. When I last did something like this I implemented a recursive descent parser. Be warned that this can be difficult to implement.

I would recommend reading: http://en.wikipedia.org/wiki/Recursive_descent_parser

And looking into the Boost Spirit parser library.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the tips. I'm fairly new too coding and using namespace std; was just something my intro class added in as a starting standard code. \$\endgroup\$ – Styx Dec 24 '14 at 4:29
  • \$\begingroup\$ @Styx, I see that a fair bit, it just happens to be something that can turn into a bad habit if you use it for any real projects. \$\endgroup\$ – shuttle87 Dec 24 '14 at 4:30
11
\$\begingroup\$

I'll mention some additional points that haven't been mentioned yet, but this does look like it can still be cleaned out quite a bit.

  • You could define those functions above main() so that prototypes won't be needed. Otherwise, put them above main(), as already mentioned in another answer.

  • These:

    #include<iostream>
    #include<cmath>
    #include<cstdlib>
    

    would look nicer with spaces:

    #include <iostream>
    #include <cmath>
    #include <cstdlib>
    

    Regarding spacing, you have some inconsistent spacing around the cases. Try not to neglect this as it could make your code more awkward to read for others.

  • type, x, and y are all declared outside of the loop, but are only used inside of the loop. In order to help keep track of them, declare them right where they're first used. Doing this with every variable can help with maintaining cleaner code, especially with larger programs.

  • This is not doing what you expect:

    while (x != 0, x--)
    

    The comma operator returns the result of the last expression, which is the x--. The other expressions are evaluated (from left to right) but the results of the expression are ignored. Thus, the result of x != 0 is evaluated with no side affects and the result dropped. This makes the expression equivalent to while (x--).

  • You don't check for division by 0, so your program can break if this operation is done. You may throw an exception for that and handle it, so that you can get a different input. Just make sure this division doesn't happen.

  • You don't need return 0 at the end of main(). The compiler will do this return for you after reaching this point.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 Division by zero is a good point to mention, I totally missed that when I first glanced at the code. \$\endgroup\$ – shuttle87 Dec 24 '14 at 4:48
2
\$\begingroup\$

Just a minor thing:

Instead of:

    total = addition(num1, num2);
    cout << num1 << " + " << num2 << "= " << total << endl;
    break;

you can make it like this:

case '+':
    cout << num1 << " + " << num2 << "= " << addition(num1, num2) << endl;
    break;
\$\endgroup\$
  • \$\begingroup\$ This may not work if the OP is trying to keep a running total. To be fair, it took me a while to find total hidden behind the lines of prototypes. \$\endgroup\$ – Jamal Dec 24 '14 at 5:12
  • \$\begingroup\$ probably, depend on him if he considered what @shuttle87 recommend, for sure he needs to keep total variable \$\endgroup\$ – MORTAL Dec 24 '14 at 5:31
  • \$\begingroup\$ @MORTAL as the program is currently programmed your improvements actually do simplify the code in a necessary way. Total will be needed after the code as implemented some of the improvements. \$\endgroup\$ – Styx Dec 24 '14 at 5:59

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.