Thread: help with getting functions to work together

ok i posted something similar earlier but i've refined it and it works almost how i want, but not quite there. i know the code is messy but that's not the issue right now. i'm trying to set a menu so that a user can choose 4 shapes to calculate the surface area of. i have to use functions for the menu, the surface area, and the "getvalue" thing. that was giving me issues before but i've gotten that working. so the problem i'm having now is that when i run the program, the menu will display, and will display again after each shape, but the input for the menu does nothing. so if sphere is 1, prism is 2, cone is 3, pyramid 4, entering any of those does nothing. and entering 5 doesn't exit my while loop. it just runs through the shapes 1, 2, 3, 4 and then back to the top. what am i doing wrong? i just need to get the menu function, and the number the user inputs in there to work with the other functions in the program.

Code:

#include <iostream>
#include <iomanip>
#include <string>
#include <math.h>
using namespace std;
const double PI = 3.14159;
int value,choice, radius;
float surface_area_sphere, surface_area_prism, surface_area_cone, surface_area_pyramid;



int menu()
{
cout<<"\nSurface Area Calculator" 
<<"\n1) Sphere"
<<"\n2) Cone"
<<"\n3) Rectangular Prism"
<<"\n4) Pyramid"

<<"\n\n5) Quit"
<<"\n\nEnter Your Choice: ";
cin>>choice;
while (choice<1 || choice>5)
{
cout<<"\nInvalid choice, please try again";
cin>>choice;
}

return(0);
}




float calcSphere (int radius)
{

surface_area_sphere=(4*PI*radius*radius);
cout<<"The surface area of the sphere is"<<fixed<<setprecision(4)<<surface_area_sphere;
return (surface_area_sphere);
}




int getvalue (string prompt, int lowerBound, int upperBound)
{
cout<<prompt<<lowerBound<<" - "<<upperBound;
cin>>value;
while (value<lowerBound || value>upperBound)
{
cout<<"invalid number, please try again";
cin>>value;
return (value);
}
}



float calcPrism (int length, int width, int height)
{
surface_area_prism = ( 2 * length * width ) + ( 2 * width * height ) +
                                    ( 2 * length * height );
cout<<"\nThe surface area of the prism is:"<<fixed<<setprecision(4)<<surface_area_prism;
return(surface_area_prism);	   	   	   	   	   	   	   	   	   
}



float calcCone(int radius, int length)
{
surface_area_cone = ( PI * radius * length ) + ( PI * radius*radius );
cout<<"\nThe surface area of the cone is:"<<fixed<<setprecision(4)<<surface_area_cone;
return (surface_area_cone);
}



float calcPyramid (int length, int height)
{
surface_area_pyramid = (2*length*height) + (length*length);
cout<<"\nThe surface area of the pyramid is: "<<fixed<<setprecision(4)<<surface_area_pyramid;
return (surface_area_pyramid);
}


int main()
{

while (choice !=5)
{
menu();


if (choice=1) 
{
value=getvalue("\nenter the radius of the sphere", 1, 10);

calcSphere (value) ;
}
menu();
if (choice=2)
{
value=getvalue("\nenter the length of the prism", 1, 20);
value=getvalue("\nenter the width of the prism", 1, 20);
value=getvalue("\nenter the height of the prism", 1, 20);
calcPrism (value, value, value);

}
menu();
if (choice=3)
{
value=getvalue("\nenter the radius of the cone", 1,7);
value=getvalue("\nenter the length of the cone", 1, 12);
calcCone (value, value);
}
menu();
if (choice=4)
{
value=getvalue("\nenter the length of the pyramid", 1, 10);
value=getvalue("\nenter the height of the pyramid", 1, 15);
calcPyramid (value, value);
}
}
return 0;
}

First of all, messy code is always an issue. If your code is messy, this hurts your ability to find a fix bugs (such as the one you're struggling with now).

The comparison operator is "==", not "=".

Code:

if (choice=1) 
...
if (choice=2)
...

Remember what I said about = and == ? This is what I was talking about.

Yes it is making a difference. You just aren't paying close enough attention.

Go though your main function on paper (or typing). Write out the program flow step by step.

You have some serious logic issues.

You are making a big mistake thinking that there is something more important to do to this code beyond indent it. The absolute #1 thing I would do first with this if I had to work on it would be to indent it.

This code could be so full of bugs that indenting it is a waste of time, because you might as well just throw it out. However, hopefully that is not the case, so you might as well indent it properly now.

Also, keeping lines down to a few hundred characters is a good idea.

You contention that "i'm able to keep track of what's happening" is TOTALLY IRRELEVANT. Look at it this way: You will be changing your mind (unless you give up programming in the next week or so). After that, you will write indented code with reasonable length lines.

So -- HEY -- let's start right now. Why not?

Code formatting is the number one way to avoid simple errors. Don't ever underestimate the value of well formatted code. When looking at older code that isn't quite formatted the way I'm used to seeing it the first thing I do is reformat it so I can read and follow it easier.

Seems to me that this could be solved by a simple interface:

Code:

...
typedef std::vector<float> dimensionData;

class IShape
{
   public:
        virtual ~IShape() { }
        virtual float CalculateArea(const dimensionData &dimensions) = 0;
};

Each shape implementation would understand how to calculate it's own area. So long as the right number of parameters were passed in depending on the shape this would work. I used a vector b/c CalculateArea() does not conform to a common interface function since certain shapes require more or less info than others to calculate their area. But who knows better than the shape itself what parameters it will expect to see in the vector?. Ordering would also be important but so long as an agreement is made between the calling code and the CalculateArea() method of the class on the order of the parameters then this won't matter. This interpretation of the various vector elements would be hard-coded in the CalculateArea inside of the derived IShape class - but since this behavior is only inside of the IShape derived class I do not see a problem with it. I'm not sure what you are allowed to do in your assignment but you should always code to an interface.

Instead of writing a function for every type of shape you would write a class and a corresponding CalculateArea() that would know how to use the data in the vector being passed to it. All that is left is to create the shape based on input, push the data entered for the dimensions into the vector and pass the vector to IShape::CalculateArea().