CodeGuru Home VC++ / MFC / C++ .NET / C# Visual Basic VB Forums Developer.com
Page 1 of 2 12 LastLast
Results 1 to 15 of 28
  1. #1
    Join Date
    Feb 2013
    Posts
    10

    [Advice Needed] Noob Code!

    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.

    Code:
    #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.

  2. #2
    GCDEF is offline Elite Member Power Poster
    Join Date
    Nov 2003
    Location
    Florida
    Posts
    12,635

    Re: [Advice Needed] Noob Code!

    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.

  3. #3
    Join Date
    Feb 2013
    Posts
    10

    Re: [Advice Needed] Noob Code!

    Alright, thanks for the reply. Is
    Code:
    int AreaTrianlge(short int Height, short int Base)
    {
        return((Height*Base)/2);
    }
    more what you mean?

  4. #4
    GCDEF is offline Elite Member Power Poster
    Join Date
    Nov 2003
    Location
    Florida
    Posts
    12,635

    Re: [Advice Needed] Noob Code!

    Quote Originally Posted by Kegs View Post
    Alright, thanks for the reply. Is
    Code:
    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.

  5. #5
    2kaud's Avatar
    2kaud is offline Super Moderator Power Poster
    Join Date
    Dec 2012
    Location
    England
    Posts
    7,822

    Re: [Advice Needed] Noob Code!

    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

    Code:
    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.

    Code:
    #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?

  6. #6
    Join Date
    Feb 2013
    Posts
    10

    Re: [Advice Needed] Noob Code!

    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

  7. #7
    GCDEF is offline Elite Member Power Poster
    Join Date
    Nov 2003
    Location
    Florida
    Posts
    12,635

    Re: [Advice Needed] Noob Code!

    Quote Originally Posted by Kegs View Post
    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.

  8. #8
    2kaud's Avatar
    2kaud is offline Super Moderator Power Poster
    Join Date
    Dec 2012
    Location
    England
    Posts
    7,822

    Re: [Advice Needed] Noob Code!

    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++ ?

  9. #9
    Join Date
    Feb 2013
    Posts
    10

    Re: [Advice Needed] Noob Code!

    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.

  10. #10
    Join Date
    Jul 2005
    Location
    Netherlands
    Posts
    2,042

    Re: [Advice Needed] Noob Code!

    Quote Originally Posted by Kegs View Post
    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.
    Cheers, D Drmmr

    Please put [code][/code] tags around your code to preserve indentation and make it more readable.

    As long as man ascribes to himself what is merely a posibility, he will not work for the attainment of it. - P. D. Ouspensky

  11. #11
    Join Date
    Feb 2013
    Posts
    10

    Re: [Advice Needed] Noob Code!

    Alright, thanks for the this. However I'm not quite sure I get what you mean. Do you mean I should do this:
    Code:
        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
    Last edited by Kegs; February 25th, 2013 at 01:36 PM.

  12. #12
    2kaud's Avatar
    2kaud is offline Super Moderator Power Poster
    Join Date
    Dec 2012
    Location
    England
    Posts
    7,822

    Re: [Advice Needed] Noob Code!

    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?

  13. #13
    Join Date
    Jul 2005
    Location
    Netherlands
    Posts
    2,042

    Re: [Advice Needed] Noob Code!

    Quote Originally Posted by 2kaud View Post
    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
    Code:
    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.
    Cheers, D Drmmr

    Please put [code][/code] tags around your code to preserve indentation and make it more readable.

    As long as man ascribes to himself what is merely a posibility, he will not work for the attainment of it. - P. D. Ouspensky

  14. #14
    Join Date
    Feb 2013
    Posts
    10

    Re: [Advice Needed] Noob Code!

    Right, so I've had a bit more of free time so I decided to learn some more. I've learnt about basic classes and the switch method which was previously told here. I have a couple of questions:
    Firstly, is this better written code? Is there anything in which I still need to improve on.
    Secondly, I'm using Code::Blocks to write my code and when I click 'New' there is an option for class. (I'm not sure if this will be detailed in a later chapter), however would it be possible to create my Class there and refer to it in my main.cpp?



    Code:
    #include <iostream>
    
    class FindingArea
    {
        public:
            double Height, Base, Radius, TopParallel, BottomParallel;
    
            double Triangle(double Height, double Base)
            {
                return((Height*Base)/2);
            }
    
            double Rectangle(double Height, double Base)
            {
                return(Height*Base);
            }
    
            double Circle(double Radius)
            {
                return(PI*(Radius*Radius));
            }
    
            double Trapezium(double Height, double TopParallel, double BottomParallel)
            {
                return((Height*(TopParallel+BottomParallel))/2);
            }
    
        private:
            double PI = 3.14159;
    };
    
    int main()
    {
        int ChoosenShape;
        FindingArea Area;
    
        std::cout << "Please choose a shape: " << std::endl << "1.Triangle, 2.Rectangle, 3.Circle, 4.Trapezium or 5. Parallelogram." <<
        std::endl << "Do this by typing the number of the shape." << std::endl;
        std::cin >> ChoosenShape;
    
        switch (ChoosenShape) {
    
        case 1: //If Triangle is choosen
            double Height, Base;//Variables for Triangle
    
            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 << std::endl << "The answer is: ";
            std::cout << Area.Triangle(Height, Base) << std::endl;
            break;
    
        case 2: //If Square is choosen
            double Width;//Variables for Square
    
            std::cout << std::endl << "You have choosen 'Rectangle'" << std::endl;
            std::cout << "Please enter in the height of the Rectangle:\t";
            std::cin >> Height;
            std::cout << "Please enter in the base of the Rectangle:\t\t";
            std::cin >> Width;
            std::cout << std::endl << "The answer is: ";
            std::cout << Area.Rectangle(Height, Width) << std::endl;
            break;
    
        case 3: //If Circle is choosen
            double Radius;//Varable for Circle
    
            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 << Area.Circle(Radius) << std::endl;
            break;
    
        case 4: //If Trapezium is choosen
            double TopParallel, BottomParallel;//Varaibles for Trapezium
    
            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 << std::endl << "The answer is: ";
            std::cout << Area.Trapezium(Height, TopParallel, BottomParallel) << std::endl;
            break;
    
        case 5: //If Parallelogram is choosen
            std::cout << std::endl << "You have choosen 'Parallelogram'" << std::endl;
            std::cout << "Please enter in the height of the Parallelogram:\t";
            std::cin >> Height;
            std::cout << "Please enter in the base of the Parallelogram:\t\t";
            std::cin >> Width;
            std::cout << std::endl << "The answer is: ";
            std::cout << Area.Rectangle(Height, Width) << std::endl;
            break;
    
        default:
            std::cout << std::endl << "Please enter a valid shape number!" << std::endl;
            break;
        }
    
        return 0;
    }
    Thanks for any information and feedback you have given and will give.

  15. #15
    Join Date
    Aug 2009
    Posts
    440

    Re: [Advice Needed] Noob Code!

    The main problem I have with your class is that it doesn't make much sense to me. I understand what you are doing with the class, but it's backwards. Your class does one thing, and that's find the area of some shapes. Now, typically, you'd make a basic shape class. From this class you can create various child/derived classes such as: Triangle, Rectangle, Circle, Trapezium, etc. Each of these classes would derive from your base shape class. Then, each of these classes would have their own area function (as well as other functions). Within main you'd get the type of shape, the basic data, and create an object for that shape and then just call its area function.

    As for Code::Blocks, creating a new class that way will add two new files to your project. You'll have a .h file where you define your class, then you'll have .cpp file where you actually implement the class. Once you get that done, you can then include the header for the class in your main.cpp file and use the class accordingly.

Page 1 of 2 12 LastLast

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  





Click Here to Expand Forum to Full Width

Featured