How good is this code?

[cplusfreak]

#include<iostream>

int add(int a, int b) {
    int sum;
    sum=a+b;
    return sum;
}

int sub(int a, int b) {
    int sub;
    sub=a-b;
    return sub;
}

int mul(int a, int b) {
    int mul;
    mul=a*b;
    return mul;
}

float div(float a, float b) {
    float div;
    if (b==0) {
        return 0;
    }
    else {
        div=a/b;
    return div;
    }
}

int main() {
    int num1, num2;
    char ope,cont;
    std::cout<<"*******************************************************************************\n"
             <<"***                                                                         ***\n"
             <<"***                  Calculator, by Shubham Chaudhary                       ***\n"
             <<"***                                                                         ***\n"
             <<"*******************************************************************************\n\n";
    std::cout<<"Input the Calculation you want to make. Ex: (3+3), (4*5), (5-3), (6/2).\n\n"
         <<"Input: ";
    std::cin>>num1>>ope>>num2;
    switch(ope) {
        case '+' :
            std::cout<<"\n"<<num1<<'+'<<num2<<'='<<add(num1, num2);
            break;
        case '-' :
            std::cout<<"\n"<<num1<<'-'<<num2<<'='<<sub(num1, num2);
            break;
        case '*' :
            std::cout<<"\n"<<num1<<'*'<<num2<<'='<<mul(num1, num2);
            break;
        case '/' :
            if (div(num1, num2)==0) {
                std::cout<<"0 as Divsor not Allowed!";
                break;
            }
            else {
            std::cout<<"\n"<<num1<<'/'<<num2<<'='<<div(num1, num2);
            break;
            }
        default :
            std::cout<<"Unknown/Invalid Operation! Try Again!";
            break;
    }
    std::cout<<"\n\nDo you want to continue(y/n)?\n"
         <<"Input: ";
    std::cin>>cont;
    if (cont=='y' || cont=='Y') {
        std::cout<<"\n\n";
        main();
    }
    else
    return 0;
}

Can anyone tell me how good and efficient is this code??? I is my project for the term and I want to confirm I can get a good grade!

[thines01]

Well, for the sake of efficiency, there is no need to declare a variable when you are just going to return a value that is easily reachable:

int add(int a, int b)
{
   return a+b;
}

The same goes with the rest of the functions like that (unless it is a requirement to do so).

If you add the using namespace std;
after your include, you can cut down on a lot of syntax (putting std:: in front of cin and cout).

Two small changes will really update your code. :)

[nullptr]

The recursive call to main() at line 71 is not a very good idea.
You'd be better off using a do/while loop.

do
{
    // main code body

    std::cin >> cont;        
} while (cont != 'y' || cont != 'Y');

return 0;

[TrustyTony]

Also looks like your code fails to calculate 0/3

Additionally you are defining div wrong with float and calling with int.

@nullptr: it should be == in continuation condition, not or, as at least I can not find way to make cont to equal both 'y' and 'Y' at the same time to exit loop.

[FelineHazard]

Also, you'd probably want to send a warning or something when user tries division by zero (lines 23-24)

[nullptr]

@pyTony, TY yes you're absolutely right. Corrected code -

do
{
    // main code body

    std::cin >> cont;        
} while (cont != 'y' && cont != 'Y');

return 0;

[TrustyTony]

Still corrected my suggestion as response is to continue not to break out, so or but == to require y for continueing.

I do not say it is good user interface, though, to ask that after every calculation. visual c++ 2008 also complained on name div and I defined it as div_ (with correct types)

Additionally program crashes if given () in input like told in instructions to user

(3+3)

[nullptr]

yeah, I need to go to bed, obviously what I wrote breaks the loop when pressing y or Y.

    do
    {
        // main code body

        std::cin >> cont;        
    } while (cont == 'y' || cont == 'Y');

[ravenous]

There are actual standard library functors to do these things for you, in <functional>. You can do things like:

#include <functional>
#include <iostream>

int main()
{
    int a = std::plus< int >()( 2, 3 );
    std::cout << a << std::endl;

    return 0;
}

In general, you should try and use standard components when you can, but I suspect that you probably have to write your own in this case; since it's an assignment.

Another thing that you should always try and avoid is repeating code. If you find yourself using cut & paste to do something, them you're probably repeating yourself :) In your example, this is definitely the case in your switch-case statement. I'd recommend doing something more like:

double result;
switch ( op )
{
    case '+' :
        result = std::plus< double >()( n1, n2 );
        break;
    case '-' :
        result = std::minus< double >()( n1, n2 );
        break;
    case '*' :
        result = std::multiplies< double >()( n1, n2 );
        break;
    case '/' :
        if ( n2 == 0 )
            // Handle this...
        else
            result = std::divides< double >()( n1, n2 );

        break;
    default :
        // Do things here...
}

std::cout << n1 << " " << op << " " << n2 " = " << result;

This way, there's no repetition; all the lines do something different.

[StuXYZ]

There are several things I would like to add : ( stuff I would mark you down on (or more likely I would give extra credit for) ).

First off, there is not such thing as a guarenteed zero float. If you write this

float div(float A,float B)
   {
     if (B==0)
       {
          // do stuff
       }
     else
       {
          return A/B;
       }
   }

You have some problems, first off is that very often it is possible to get NEARLY zero, (depending on your system), you are going to get small rounding errors at the 1e-38 level, they are NOT zero. You also have the problem, what happens when (consider that float max is 1e38), you divide say 1e30/1e-20, you now have a number that has overflowed. (or the reverse 1e-20/1e30 goes to zero.) How you handle it is up to you, but something telling me what you expect to happen is good. [even a comment -- e.g. // over/underflows are user reponsibilty]

Then you do something very strange, you input int s but pass floats (by implicit conversion) to divide. Why not pass int s and then allow float to convert, that way you will have a robust zero test. E.g

 float div(int a,int b)
   {
      if (b==0)
        {
          std::cerr<<"Division by zero is forbidden in this program"<<std::endl;
          return 0;
        }
      return static_cast<float>(a)/static_cast<float>(b);
   }

Note the use of static_cast<float> to convert the ints to float. If you don't and you do just a/b you will find stuff like 6/7 returning 0, as the conversion to a float happens after the division.

[thines01]

Also: by the PC Lint rules, you should initialize all variables when they are instantiated.

int num1=0;
int num2=0;
char ope='\0';
char cont='\0';