5
\$\begingroup\$

My program is a fraction calculator that is supposed to calculate the basic operations (+, -, *, /) and is supposed to exit when I enter % as my sentinel value. This is what I have so far; any feedback is highly appreciated.

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






enum MenuSelection{

NONE      =0,
ADD       =1,
SUBTRACT  =2,
MULTIPLY  =3,
DIVIDE    =4,
QUIT      =5,
END       =6,






};



int menu()
    {  
    int MenuSelection=0;

do  {


cout<<"\n1) Add"<<endl;
cout<<"2) Subtract"<<endl;
cout<<"3) Multiply"<<endl;
cout<<"4) Divide"<<endl;
cout<<"5) %"<<endl;

cin>> MenuSelection;
//
 if(MenuSelection<= NONE || MenuSelection >= END)
cout<<"Please pick a number from 1-5 as shown in the menu selection."<<endl;


if(MenuSelection==QUIT)
{cout<<"You have chosen to exit the program"<<endl; 
    cout<<"You have answered \n";

        exit(1);


    }

}

while(MenuSelection<= NONE || MenuSelection >= END);



//
return MenuSelection;



}








void addFractions(int num1, int num2, int den1, int den2, int &NUM_result, int    &DEN_result)
{        //
    NUM_result= (num1* den2) +(num2*den1);
     //
    DEN_result= den1*den2;


}

void subFractions(int num1, int num2, int den1, int den2, int &NUM_result, int &DEN_result)
{   NUM_result = (num1*den2) - (num2*den1);
   //

DEN_result= den1*den2;

   //


}

void multiplyFractions(int num1, int num2, int den1, int den2, int &NUM_result, int &DEN_result)
{   NUM_result = num1*num2;
//

DEN_result= den1* den2;

   }

void divideFractions(int num1, int num2, int den1, int den2, int &NUM_result, int &DEN_result)
{            //

     NUM_result = num1*den2;
         //
     DEN_result = den1*num2;

 }





 void input_values_FROM_USER(int &num1, int &num2, int &den1, int &den2)

{ 


cout<<endl;
cout<<"Enter the numerator for first fraction-->";
cin>>num1;
cout<<"Enter denominator for first fraction--> ";
cin>>den1;
cout<<"Enter numerator for second fraction--> ";
cin>>num2;
cout<<"Enter denominator for second fraction--> ";
cin>>den2;
cout<<"-------------------------------------------\n\n\n";


  }






void outputResults(int num1, int num2, int den1, int den2, int &NUM_result, int &DEN_result, int operation)
{
    if(operation == ADD)
{ cout<<"The sum of the two is--> "<<num1<<'/'<<den1<<'+'<<num2<<'/'<<den2<<'='<<NUM_result<<'/'<<DEN_result<<endl;
}


if(operation== SUBTRACT)
{ cout<<"The difference of the two is--> "<<num1<<'/'<<den1<<'-  '<<num2<<'/'<<den2<<'='<<NUM_result<<'/'<<DEN_result<<endl;

}

if(operation== MULTIPLY)
{
cout<<"The product of the two is--> "     <<num1<<'/'<<den1<<'*'<<num2<<'/'<<den2<<'='<<NUM_result<<'/'<<DEN_result<<endl;
}


 if(operation== DIVIDE)
 {
 cout<<"The quotient of the two is--> "  <<num1<<'/'<<den1<<'/'<<num2<<'/'<<den2<<'='<<NUM_result<<'/'<<DEN_result<<endl;
     }
  }





 void main()
 { int MenuSelection = NONE;
int num1=0, num2=0, den1=0, den2=0;
int NUM_result=0, DEN_result=0;


cout<<"Author: Jose Soto."<<endl;
cout<<"CSCI 110-- Jose Soto's Project 2 (Basic Fraction Arithmetic)\n\n"<<endl;
do
{ MenuSelection= menu();

  input_values_FROM_USER(num1,num2,den1,den2);

if(MenuSelection==ADD)

    { addFractions(num1,num2,den1,den2,NUM_result, DEN_result);
    }

if(MenuSelection==SUBTRACT)
    { subFractions(num1,num2,den1,den2,NUM_result, DEN_result);

    }


if(MenuSelection==MULTIPLY)
{ multiplyFractions(num1,num2,den1,den2,NUM_result,DEN_result);

}

if(MenuSelection==DIVIDE)
{ divideFractions(num1,num2,den1,den2,NUM_result,DEN_result);

}



outputResults(num1,num2,den1,den2,NUM_result,DEN_result,MenuSelection);
}

while(MenuSelection != QUIT);


cin.get();



}
\$\endgroup\$
5
  • 2
    \$\begingroup\$ Welcome! Please consider using proper, consistent indentation and avoiding excess whitespace. It's all over the place here and is hard to read. \$\endgroup\$
    – Jamal
    Jan 27, 2014 at 6:43
  • 1
    \$\begingroup\$ Your original question didn't match the code you presented. We can't really review code that you intend to write, only code that you have already written, so I've edited the question accordingly. \$\endgroup\$ Jan 27, 2014 at 6:58
  • \$\begingroup\$ I am supposed to have the number of correct out of the total answered. For example when I enter %, it is supposed to display something like "You answered 8 out of 10 questions correctly, you have 80.5% which is a letter grade of B." Then it's supposed to exit the program. I'm not too sure what to write in terms of that. \$\endgroup\$ Jan 27, 2014 at 7:02
  • 2
    \$\begingroup\$ In Code Review, our minimum standard is that we review code that already works. (See help center.) Would you like a critique of the working features of your code, or would you prefer to withdraw the question until you have completed the code? \$\endgroup\$ Jan 27, 2014 at 7:11
  • \$\begingroup\$ A critique would suffice. \$\endgroup\$ Jan 27, 2014 at 8:04

3 Answers 3

6
\$\begingroup\$

Had a few thoughts on your code.

Your code doesn't accept a '%' to quit. Changing the menu to accept a '%' makes the rest of it inconsistent. so making all the input match the operation and changing the enum to reflect the character values instead of arbitrary ones helps with this.

You're parsing the menu input in 2 places. It's easier to maintain if you use the menu routine just to read the input and keep any parsing of the input in the calling routine.

A switch block improves the readability when you have to parse the user's input.

When parsing user input through a menu putting the input into a char instead of an int makes it harder for user input to break your code.

When you're sending literal strings to the console it's usually more efficient to to add a '\n' to the end of it rather than sending endl separately.

Here's some revised code which cleans up a number of issues:

#include <iostream>
#include <cstdlib>

using namespace std;


enum MenuSelection
{
    ADD       = 43,
    SUBTRACT  = 45,
    MULTIPLY  = 42,
    DIVIDE    = 47,
    QUIT      = 37,
};
void input_values_FROM_USER(int &num1, int &num2, int &den1, int &den2)

{ 
    cout<<endl;
    cout<<"Enter the numerator for first fraction-->";
    cin>>num1;
    cout<<"Enter denominator for first fraction--> ";
    cin>>den1;
    cout<<"Enter numerator for second fraction--> ";
    cin>>num2;
    cout<<"Enter denominator for second fraction--> ";
    cin>>den2;
    cout<<"-------------------------------------------\n\n\n";
}
char menu()
{  
    char MenuSelection = 0;

    cout<<"\n+) Add\n";
    cout<<"-) Subtract\n";
    cout<<"*) Multiply\n";
    cout<<"/) Divide\n";
    cout<<"%) Quit\n";
    cin>> MenuSelection;
    return MenuSelection;
}
void addFractions(int num1, int num2, int den1, int den2, int &NUM_result, int    &DEN_result)
{ 
    NUM_result= (num1* den2) +(num2*den1);
    DEN_result= den1*den2;
}

void subFractions(int num1, int num2, int den1, int den2, int &NUM_result, int &DEN_result)
{   
    NUM_result = (num1*den2) - (num2*den1);
    DEN_result= den1*den2;
}

void multiplyFractions(int num1, int num2, int den1, int den2, int &NUM_result, int &DEN_result)
{  
    NUM_result = num1*num2;
    DEN_result= den1* den2;
}
void divideFractions(int num1, int num2, int den1, int den2, int &NUM_result, int &DEN_result)
{
    NUM_result = num1*den2;
    DEN_result = den1*num2;
}
void outputResults(int num1, int num2, int den1, int den2, int &NUM_result, int &DEN_result, int operation)
{
    cout<<"The result of the operation is--> "<<num1<<'/'<<den1<<(char)operation<<num2<<'/'<<den2<<'='<<NUM_result<<'/'<<DEN_result<<endl;
}
void main()
{ 
    char MenuSelection = '0';
    int num1=0, num2=0, den1=0, den2=0;
    int NUM_result=0, DEN_result=0;
    bool bad = false;
    cout<<"Author: Jose Soto."<<endl;
    cout<<"CSCI 110-- Jose Soto's Project 2 (Basic Fraction Arithmetic)\n\n\n";
    do
    {
        bad = false;
        MenuSelection = menu();
        switch(MenuSelection)
        {
        case ADD:
            input_values_FROM_USER(num1,num2,den1,den2);
            addFractions(num1,num2,den1,den2,NUM_result, DEN_result);
            break;
        case SUBTRACT:
            input_values_FROM_USER(num1,num2,den1,den2);
            subFractions(num1,num2,den1,den2,NUM_result, DEN_result);
            break;
        case MULTIPLY:
            input_values_FROM_USER(num1,num2,den1,den2);
            multiplyFractions(num1,num2,den1,den2,NUM_result,DEN_result);
            break;
        case DIVIDE:
            input_values_FROM_USER(num1,num2,den1,den2);
            divideFractions(num1,num2,den1,den2,NUM_result,DEN_result);
            break;
        case QUIT:
            cout<<"\nYou have chosen to exit the program\n"; 
            bad = true;
        default:
            cout<<"Please pick an option as shown in the menu.\n";
            bad = true;
            break;
        }
        if(!bad)
            outputResults(num1,num2,den1,den2,NUM_result,DEN_result,MenuSelection);
    }
    while(MenuSelection != QUIT);
    cin.get();
}
\$\endgroup\$
5
\$\begingroup\$

Since you have a C++ tag on this question, I assume this is eventually meant as a C++ program. If that is the case, you should have

class Fraction {
   int denominator;
   int numerator;
   Fraction(int d, int n) : denominator(d), numerator(n) {}
   // define operators for +, -, etc.
}

If you are still writing just in C—combine parsing the user input with dispatching the appropriate message to an object.

\$\endgroup\$
3
\$\begingroup\$

Some things not mentioned:

  1. Please do not use using namespace std. Read this
  2. void main() is not a part of the C++ standard.
  3. Consider creating a class for representing fractions. The arithmetic functions should be implemented in operator overloads. Combining advice from the previous answers, your code would become easier to read and maintain.

    Note: In general, operators that return a new value should be returned by value.

        class Fraction
        {
    
        public:
            Fraction(int a,int b) : numerator(a), denominator(b){}
            Fraction operator+(const Fraction&, const Fraction&); //Return a Fraction by value
            Fraction operator-(const Fraction&, const Fraction&);
            Fraction operator*(const Fraction&, const Fraction&);
            Fraction operator/(const Fraction&, const Fraction&);
    
        private:
    
            int denominator;
            int numerator;
    
        };
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.