Welcome to the Treehouse Community

Want to collaborate on code errors? Have bugs you need feedback on? Looking for an extra set of eyes on your latest project? Get support with fellow developers, designers, and programmers of all backgrounds and skill levels here with the Treehouse Community! While you're at it, check out some resources Treehouse students have shared here.

Looking to learn something new?

Treehouse offers a seven day free trial for new students. Get access to thousands of hours of content and join thousands of Treehouse students and alumni in the community today.

Start your free trial

C#

Posted by Samuel Savaria
Samuel Savaria
4,809 Points

Geometry calculator

https://w.trhou.se/9ysov1fr4m

I did the Geometry Calculator from C# Objects. I am mostly looking for feedback on how to keep my code dry, and how to make it more readable.

2 Answers

Allan Clark
Allan Clark
10,810 Points

Now I am not sure what the instructions for the calculator are specifically but i have a few pointers just based on what you have.

First, try to keep you Main() method to a minimum. if there is more than trivial logic it should be in its own method at the very least (better if its in another class I dont like having other methods on the Main.cs to keep the whole file as minimal as possible).

Secondly, this is a very procedural approach, the classes you have are static helper classes that handle the computations and the main method handles the interaction with the user. Try to think about the objects involved, namely triangles, circles, and rectangles (being squares are rectangles you can use some inheritance magic here). Try creating classes that model objects using the properties as the dimensions and the methods to do all the computations.

Lastly, 'while(true)' is valid but bad form, you will be able to find better ways to control the logic flow (ones that don't have inherent risks of an infinite loop : ) ). Console.ReadLine() should wait for input anyway so they may not be needed.

Allan Clark
Allan Clark
10,810 Points

Oh and if you want bonus style points to try using TryParse() methods instead of plain ole Parse().

Samuel Savaria
Samuel Savaria
4,809 Points

In my next post I was about to ask if, ideally, all user inputs should be in main and the actual coding in classes. That answers my question perfectly. One thing tho, can you use System.Console.ReadLine() in a static class? The majority of my code in pretty much every of my projects were taking the input of the user, and that's prevented me from keeping my code dry. If I could assign the value of X to the result of getNumber, I will save a ridiculous amount of space.

Thanks for the .TryParse method, will certainly try it out!

Now, the other big question. I mostly used while(true) in user inputs and for loops, because it allows me to repeat the loop without passing an invalid input or to run the same i value again. Do you know how I can get around that?

Thanks a lot, my objective for after the C# Intermediate course was to keep my code as dry as possible. This is bringing me a lot closer to it.

Allan Clark
Allan Clark
10,810 Points

Yes you can certainly use System.Console.ReadLine() in a static class, as it is a static method.

I threw together a 'GeometryCalculator' class to give more of an OO approach. Forewarning there may be some more advanced stuff than where you are in the Treehouse courses, feel free to throw out any questions you have about it. And by no means am I saying this is the best or most efficient way to do it but this will show a more OO way of thinking about the solution.

The idea of this class is to encapsulate the functionality of the calculator itself. This class assumes there are Triangle, Cirlcle, Rectangle, and Square classes that inherit from a base Shape class.

With this new class you're Main method will go down to 2 lines.

var calculator = new GeometryCalculator();
calculator.DisplayWelcome(); //of course variable and method names can be changed to your preference
Allan Clark
Allan Clark
10,810 Points 
public class GeometryCalculator
    {
        private string WelcomeInstructions = "Enter \"triangle\", \"rectangle\", \"square\" or \"circle\", or type \"quit\" to leave: "; 

        public void DisplayWelcome()
        {
            do
            {
                Console.Write(WelcomeInstructions);
                var entry = Console.ReadLine();
                Shape selectedShape = ParseShape(entry);

            } while (selectedShape != null);

            GetDimensionsFromUser(selectedShape);
            //add a method here to display the calculations, this can be done with another private method on this class or in methods on the shapes or a combination of both all up to you

            DisplayWelcome(); // this will start the whole process over again essentially keeping the program running until the user inputs 'quit' 
        }

        public Shape ParseShape(string entry)
        {
            entry = entry.Trim().ToLower(); //gaurantee no lead/trail white space and all lower
            if(entry == "triangle")
            {
                return new Triangle();
            }
            // etc
            if(entry == "quit")
            {
                System.Environment.Exit(0);
            }
        } 

        public void GetDimensionsFromUser(Shape selectedShape)
        {
            Triangle tri = selectedShape as triangle; 
            if(tri != null)
            {
                tri.GetBaseHeight(); 
            }

            Rectangle rec = selectedShape as Rectangle;
            if(rec != null)
            {
                rec.GetSides();
            }

            Square sq = selectedShape as Square;//though can do some improvement here if you use inheritance and have a Square object just be a Rectangle object with equal sides
            if(sq != null)                      //possibly the Rectangle has a method or property called 'IsSquare' that returns a bool
            {
                sq.GetSide();
            }

            Circle cir = selectedShape as Circle;
            if(cir != null)
            {
                cir.GetRadius(); //Remember radius is twice diameter, so we should prompt for (and store) only one since we can calculate the other on the fly with a property
            }
        }
Samuel Savaria
Samuel Savaria
4,809 Points

I don't think I had any problem with OOP, I was simply trying to solidify what was being passed over quickly in the course. I understand that ideally, the main() would only call other classes and never run any of the code itself. But a lot was unclear after the course and I wanted to make sure I understood and knew how to use everything before moving on. My objective for next course is to keep code learned in the first 2 courses 100% dry.

Allan Clark
Allan Clark
10,810 Points

Well, for an OO approach your code is conspicuously missing instantiating those objects using the 'new' keyword, and relies heavily on Static methods on Static classes (which more procedural than OO). If you have questions about topics that may be unclear, this forum is great for asking specific questions. If you post code and ask for improvements you may get an answer back that does not answer your specific questions and may lead to more confusion.

I am glad I was able to clear up a few things for you! Please post any other questions or issues you run into as you move through the course.

Samuel Savaria
Samuel Savaria
4,809 Points

https://w.trhou.se/httav2ykva

This is my "big" project that I hope uses a more OO approach. It's very, very, very messy, maybe 80%+ of the code is in the Main() method. I was planning on drying it up a bit but I injured my arm recently and didn't feel like having to write too much code, so I just wrote on top of the existing mess :P

Also, ignore that pointless AI class with only a RNG, I started to code it before my injury and had trouble calling in the ships array from the Main(), then I just didn't care enough to look it up. Bad coding practice, but I went from 5h+ of coding per day to 20-30 minutes, so I didn't feel like trying owt fancy.

Allan Clark
Allan Clark
10,810 Points

I am sorry to hear that and wish you a speedy recovery! Life > code haha

Anyway as for the code, this is definitely heading in the right direction. The Objects you have are modeled well and encapsulate the functionality pretty well. Couple pointers:

  • class name and file name should match
  • one class per file *Note this is a general rule so it does have its exceptions. Grouping the custom exceptions together should be done with a folder. While they are small classes and it would be fine in this instance, but its best practice for larger projects
  • It could use a couple more classes to encapsulate most of whats is going on int the Main() method.
  • when creating a ship instead of copying the values from the MapLocation object, just store the whole MapLocation as a custom field. That way it will read a little better when dealing with the Ship Object. 'ship.Location.x' is much more clear than 'ship.x'.
  • use names like 'isTouching' only for boolean values (this is actually used as a bool later in the code just with 1 and 2 instead of true and false)

I would start with dealing with the structure fist. Create a Program.cs that contains only the Program class and Main() method. Look at what is in your Main() method as a large group of properties and methods that need to be sorted into categories.

Have your Game.cs/class as the engine of the game. It should have stuff like Players (also another object id suggest), a Map (same thing here), etc.

  • Game has a Map
  • Map has Ships
  • Ship has a MapLocation
Samuel Savaria
Samuel Savaria
4,809 Points

I will apply all of your suggestions on my next project for sure :) I'd like to add that isTouching was originally a bool, but because ships couldn't be on top of each other I applied lazy design and changed it to an int.

Also, you say there should be 1 class per file, but is it a personal preference? In my opinion, it makes a lot more sense to have the class constructor (Ship, MapLocation) and then any methods related to it (Fire, GetLocation, OnMap, etc..). That way, files act as a sort of category and makes searching for methods a lot easier.

Allan Clark
Allan Clark
10,810 Points
Allan Clark
Allan Clark
10,810 Points

One class per file is pretty standard. But the only file you have with multiple classes is the Exception.cs. This would be better in two separate files (BattleshipException.cs and OutOfBoundsException.cs) with those files inside a folder called 'Exceptions'. Like i said it is ok here on this small app but when you get to more complex applications you could have a ton of custom exceptions and if they were all on one file and you could have to scroll through hundreds of lines of code to find what you are looking for. This is especially important when you are working with a team and other people will be developing on the same project.

The data model being a little lacking is causing you to have to resort to a procedural thinking for some of the concepts. (for example about half of MapLocation should be in a new Map class) Yes a .cs Class file will generally have a constructor, Variables (for object state), and Methods (for object behavior). 'Object behavior' is the key here. Methods that are just related to the object is too broad, methods represent what an object does, or has done to it.

A good example of this is the Ship class and the Fire() method. While yes Fire() is related to a ship object, but if you think about the physical battleship board game, the ship really isn't the one doing the firing action (the player does).

And take all this just as pointers, it will become more clear as you get more experience with it and see more existing code.