[Advice Needed] Noob Code!

[Kegs]

Hello all, I recently began to learn C++ from reading a book. I'm up to chapter 4 of 24 and thought I'd like to try out what I've learnt to far. My program works fine, however I'd like some advice on the way I have written the program and whether or not it can be simplified and how. The program overall is to work of the area of some shapes. The user needs to input the shape he is using the the information required to work out the area.

#include <iostream>

int AreaTrianlge(short int Height, short int Base)
{
    std::cout << std::endl << "Working out the Area of the Triangle!" << std::endl << "The Answer is: ";
    return((Height*Base)/2);
}

int AreaSquare(short int Height, short int Width)
{
    std::cout << std::endl << "Working out the Area of the Sqaure!" << std::endl << "The Answer is: ";
    return(Height*Width);
}

float AreaCircle(float Radius)
{
    float PI = 3.14159;

    std::cout << std::endl << "Working out the Area of the Circle!" << std::endl << "The Answer is: ";
    return(PI*(Radius*Radius));
}

int AreaTrapezium(short int Height, short int TopParallel, short int BottomParallel)
{
    std::cout << std::endl << "Working out the Area of the Trapezium!" << std::endl << "The Answer is: ";
    return((Height*(TopParallel+BottomParallel))/2);
}

int main()
{
    short int ChoosenShape;
    short int Height, Base, Width, TopParallel, BottomParallel; //Variables for Area
    float Radius; //Variable for Area

    std::cout << "Please choose a shape:" << std::endl << "1.Triangle, 2.Square, 3.Circle or 4.Trapezium" << std::endl << "Do this by typing the number of the shape" << std::endl;
    std::cin >> ChoosenShape;

    if (ChoosenShape == 1)//If Triangle is choosen
    {
        std::cout << std::endl << "You have choosen 'Triangle'" << std::endl;
        std::cout << "Please enter in the height of the Triangle:\t";
        std::cin >> Height;
        std::cout << "Please enter in the base of the Traingle:\t";
        std::cin >> Base;
        std::cout << AreaTrianlge(Height, Base) << std::endl;
    }
    else if (ChoosenShape == 2)//If Square is choosen
    {
        std::cout << std::endl << "You have choosen 'Square'" << std::endl;
        std::cout << "Please enter in the height of the Square:\t";
        std::cin >> Height;
        std::cout << "Please enter in the base of the Square:\t\t";
        std::cin >> Width;
        std::cout << AreaSquare(Height, Width) << std::endl;
    }
    else if (ChoosenShape == 3)//If Circle is choosen
    {
        std::cout << std::endl << "You have choosen 'Circle'" << std::endl;
        std::cout << "Please enter in the radius of the Circle:\t";
        std::cin >> Radius;
        std::cout << AreaCircle(Radius) << std::endl;
    }
    else if (ChoosenShape == 4)//If Trapezium is choosen
    {
        std::cout << std::endl << "You have choosen 'Trapezium'" << std::endl;
        std::cout << "Please enter in the height of the Trapezium:\t\t\t\t";
        std::cin >> Height;
        std::cout << "Please enter in the Length of the Top Parallel of the Trapezium:\t";
        std::cin >> TopParallel;
        std::cout << "Please enter in the Length of the Bottom Parallel of the Trapezium:\t";
        std::cin >> BottomParallel;
        std::cout << AreaTrapezium(Height, TopParallel, BottomParallel) << std::endl;
    }
    else

    std::cout << std::endl << "Please enter a valid shape number!" << std::endl;
    return 0;
  }

Like I said before and advice on how I written the could would be amazing thanks all! P.S If this isn't the sort of post that is allowed here, then please close the thread.

[GCDEF]

I'm not crazy about the way you've done the output. You should remove all your output statements from the functions that calculate area. Put them in main with the rest of them.

[Kegs]

Alright, thanks for the reply. Is

int AreaTrianlge(short int Height, short int Base)
{
    return((Height*Base)/2);
}

more what you mean?

[GCDEF]

Alright, thanks for the reply. Is

int AreaTrianlge(short int Height, short int Base)
{
    return((Height*Base)/2);
}

more what you mean?

Yep, except int may not be the best choice for a return value. If Height * Base is an odd number, the result will be wrong.

[2kaud]

Rather than use multiple nested if statements, you could use the switch statement (though this might be covered in a later chapter in your book). Basically you code as

switch (ChoosenShape) {
     case 1:
        std::cout << std::endl << "You have choosen 'Triangle'" << std::endl;
        std::cout << "Please enter in the height of the Triangle:\t";
        std::cin >> Height;
        std::cout << "Please enter in the base of the Traingle:\t";
        std::cin >> Base;
        std::cout << AreaTrianlge(Height, Base) << std::endl;
       break;

     case 2:
       ......

     case 3:
     ......

     default:
        std::cout << std::endl << "Please enter a valid shape number!" << std::endl;
        break;
}

You can also cut down on typing and make your program slightly easier to read by removing all the std:: and inserting using namespace std; after your #include.

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

    cout << "Please choose a shape:" << endl << "1.Triangle, 2.Square, 3.Circle or 4.Trapezium" << endl << "Do this by typing the number of the shape" << endl;
    cin >> ChoosenShape;

Also where the result of a calculation is not necessarily an integer (such as area of triangle) rather than returning an int, float could be returned.

I agree with GCDEF about not putting output statements in a function. What if you wanted to use these functions when you were writing a graphical program and the console was not available? Or you wanted output to go to a file instead? A function should really do only one thing (but well!) and not have any un-expected side-effects (such as outputing text).

Question for you to consider. Why have you used short int rather than just int?

[Kegs]

Thank you both for the feedback. I have now removed the output from the functions and included the 'using namespace std;' into the code. I have used a short int instead of a int as on my computer int is bigger than an int so I did this to make the program smaller.

I'll look into the switch thing, but I'd rather wait to learn fully how to use it later on. Thanks for telling me about it though guys :P

[GCDEF]

Thank you both for the feedback. I have now removed the output from the functions and included the 'using namespace std;' into the code. I have used a short int instead of a int as on my computer int is bigger than an int so I did this to make the program smaller.

I'll look into the switch thing, but I'd rather wait to learn fully how to use it later on. Thanks for telling me about it though guys :P

Switch is really just a stylistic thing. Either is okay. As to making your program smaller, 40 years ago saving every byte possible was important, at that level, it's not something you need to be concerned with these days.

[D_Drmmr]

Thank you both for the feedback. I have now removed the output from the functions and included the 'using namespace std;' into the code. I have used a short int instead of a int as on my computer int is bigger than an int so I did this to make the program smaller.

Personally, I'd advice against putting "using namespace std;" in your code. It doesn't save much typing (which is always a bad reason, by the way), doesn't make your code that much more readable and it can be an annoying habit later on, when you will work more with header files.

Regarding types, you should default to using (unsigned) int when you need an integer type and double if you need a floating point type. Only use float instead of double if you need to save memory (or disk space) or if you have profiled your application and using float calculations is that much faster than using doubles. Otherwise, just go for the extra precision.

One thing that wasn't mentioned before, is that it's bad style to define all your variables at the beginning of a function. You should try to only define variables once you can give them a meaningful initial value.

[Coder Dave]

One thing that wasn't mentioned before, is that it's bad style to define all your variables at the beginning of a function. You should try to only define variables once you can give them a meaningful initial value.

I have to disagree with this. If you define your variables where they are first used, you end up having variable definitions all over the place. Now if you want to know the type of a variable, you have to go hunting for its definition instead of just looking at the top of the function. Also, if you use the same name to define variables in different blocks or scopes, the code becomes confusing to read. Some compilers will usually give you a warning if you use a variable in an expression before assigning any value to it and once you become experienced, assigning a value to a variable the first time it is used, becomes second nature.

[GCDEF]

I have to disagree with this. If you define your variables where they are first used, you end up having variable definitions all over the place. Now if you want to know the type of a variable, you have to go hunting for its definition instead of just looking at the top of the function. Also, if you use the same name to define variables in different blocks or scopes, the code becomes confusing to read. Some compilers will usually give you a warning if you use a variable in an expression before assigning any value to it and once you become experienced, assigning a value to a variable the first time it is used, becomes second nature.

I'm going to go with D_Drmmr on this. I think it's cleaner to define them when you use them. There's less hunting that way, and any modern IDE can show you the type and where it's defined with a mouse hover or click if you're having trouble finding it.

[2kaud]

Pleased to be of help. If you want more feedback later, or have questions about using c++ just post some code and ask. Incidentally, what book are you using from which to learn c++ ?

[Kegs]

Currently I am using. SAMS Teach Yourself C++ in 24 hours. I found it for like £3 so I thought why not. Not sure if it's a good one, but so far it looks decent.

[Kegs]

Alright, thanks for the this. However I'm not quite sure I get what you mean. Do you mean I should do this:

    else if (ChoosenShape == 3)//If Circle is choosen
    {
        double Radius;

        std::cout << std::endl << "You have choosen 'Circle'" << std::endl;
        std::cout << "Please enter in the radius of the Circle:\t";
        std::cin >> Radius;
        std::cout << std::endl << "The answer is: ";
        std:: cout << AreaCircle(Radius) << std::endl;
    }

EDIT: I have tried this out, and for each if statement, I need to reinitialize each double. For example, I do double Height, in the if Triangle bit, but then again in if Square, I need to do double Height again. Is this ok or would it be easier to just do them all in the begging like I did to begin with? Thanks

[2kaud]

Yes, that's what meant. However, as you have found, this brings about the concept of variable scope which you probably haven't come across yet in your tutorial book (look for a section(s) on scope). Basically, identifiers declared inside a block (between {..}) have block scope. Block scope begins at the identifier's declaration and ends at the terminating }. Outside of this block scope, the variable is not defined. That's why you had to define the variables in each 'bit' of your program. When they were defined at the beginning they were available to all the 'bits' but when defined in a 'bit' they are only available in that 'bit'. As Radius is only appropriate to a circle, for example, then good practice says that Radius should only be defined in the block that processes circle. Also that variables should be either initialised when defined or as soon as possible after. This is to make sure that a variable is not used before its value has been set. Some compliers on some warning levels produce a compile time message about this.

Just a further observation. The height and width of a square are the same. If they are not then it is a rectangle. So the area of a square is just width * width.

I'd advise against putting "using namespace std;" in your code ... doesn't make your code that much more readable and it can be an annoying habit later on, when you will work more with header files.

I agree that you don't put 'using ...' in any header file. However, unless you are using multiple namespaces then my personal opinion is that having 'using namespace std;' after the #include in your program and not using std:: everywhere does make the code a lot more readable - especially when later you'll start to use other standard classes like string, vector etc. What does your book have to say on this?

[D_Drmmr]

I agree that you don't put 'using ...' in any header file. However, unless you are using multiple namespaces...

So, what do you do when you introduce, say, a boost library to a cpp file that you created before? Do you remove "using namespace std;" and spend maybe 10 minutes to fix all the compiler errors? Doesn't seem like a good idea to use a coding style that is so fragile to changes.

The problem I meant with using header files is when you have class definitions or function declarations in a header file and the implementation in a cpp file. If the (member) functions have arguments or a return value that is a class in the std namespace, then the signature will look different in the header file and the cpp file. E.g. in the header file you have

std::string MyFunction(const std::vector<int>& v);

// and in the cpp file you have

string MyFunction(const vector<int>& v) {
    //...
}

If instead you don't use the "using namespace" stuff (at least, not at global scope), then the two are the same and you can just copy-paste.