10
\$\begingroup\$

I have an assignment where I am required to verify if a car is a sports car or not by minimum max speed and minimum horse power. I have finished the program and it works great. I was just wanting to see if you can review my assignment and see if there is room for improvement.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace Project3
{
class sportsCar
{
    public int _maxSpeed = 0;
    public int _horsepower = 0;
    private static int _MinimumRequiredSpeed = 150;
    private static int _MinimumRequiredHorsepower = 250;

    public bool IsSportsCar()
    {
        if ((_maxSpeed >= _MinimumRequiredSpeed) && (_horsepower >= _MinimumRequiredHorsepower))
            return true;
        else
            return false;
    }
    public void maxSpeed(int newmaxSpeed)
    {
        _maxSpeed = newmaxSpeed;
    }
    public void horsepower(int newhorsepower)
    {
        _horsepower = newhorsepower;
    }
}


class Program
{
    static void Main(string[] args)
    {
        sportsCar car = new sportsCar();

        Console.Write("What is the max speed? ");
        int newmaxSpeed = int.Parse(Console.ReadLine());
        Console.Write("What is the max horsepower? ");
        int newhorsepower = int.Parse(Console.ReadLine());

        car.maxSpeed(newmaxSpeed);
        car.horsepower(newhorsepower);

        bool sportCar = car.IsSportsCar();
        if (sportCar == true)
        {
            Console.WriteLine("This is Definately a Sports Car");
        }
        else
        {
            Console.WriteLine("This is NOT a Sports Car");
        }
    }
}
}
\$\endgroup\$
15
\$\begingroup\$

_maxSpeed and _horsePower should be private.

You have (confusingly named) setter methods for public variables, but no getters. That makes for an odd interface.

In C# you by convention use properties for getters and setters. So instead of

public int _maxSpeed = 0;
public int _horsepower = 0;
public void maxSpeed(int newmaxSpeed)
{
    _maxSpeed = newmaxSpeed;
}
public void horsepower(int newhorsepower)
{
    _horsepower = newhorsepower;
}

You have

public int MaxSpeed { get; set; }
public int HorsePower { get; set; } 

Since these values are unlikely to change, you could make the type immutable:

class Car
{
    public readonly int MaxSpeed;
    public readonly int HorsePower;

    public Car( int maxSpeed, int horsePower )
    {
        MaxSpeed = maxSpeed;
        HorsePower = horsePower;
    }
}

Instead of checking a boolean expression and then returning a literal true or false, just return the expression instead. Booleans are a type just like any other. Also, you don't need the comparison to true or false for the same reason.

if( someCondition == true )
    return true;
else
    return false;

You have

if( someCondition )
    return true;
else
    return false;

or better yet

return someCondition;

So

return _maxSpeed >= _MinimumRequiredSpeed && 
       _horsepower >= _MinimumRequiredHorsepower;

Whenever you take user input you need to check it. Your program will crash if the user enters something that is not a number. You can check this by using TryParse instead of Parse.

int horsePower;
do
{
    string input = Console.ReadLine();
}
while( !int.TryParse( input, out horsePower ) );

Other than that it is fine, keep at it ;)

\$\endgroup\$
5
\$\begingroup\$

The other answerers caught most of the issues I can see, however here are a couple of small ones:

  • In C# (well, .NET) you should follow the standard naming conventions. In particular, classes, properties and methods should be PascalCased.

  • You spelt "definitely" wrong ;)

\$\endgroup\$
5
\$\begingroup\$

Here's how I would clean it up (explanations below):

namespace Project3
{
    using System;

    internal sealed class SportsCar
    {
        private const int MinimumRequiredSpeed = 150;
        private const int MinimumRequiredHorsepower = 250;
        private readonly int maxSpeed;
        private readonly int horsepower;

        public SportsCar(int maxSpeed, int horsepower)
        {
            this.maxSpeed = maxSpeed;
            this.horsepower = horsepower;
        }

        public bool IsSportsCar()
        {
            return (this.maxSpeed >= MinimumRequiredSpeed) && (this.horsepower >= MinimumRequiredHorsepower);
        }

        public int MaxSpeed
        {
            get
            {
                return this.maxSpeed;
            }
        }

        public int Horsepower
        {
            get
            {
                return this.horsepower;
            }
        }
    }

    internal static class Program
    {
        private static void Main()
        {
            Console.Write("What is the max speed? ");
            int newmaxSpeed;
            if (!int.TryParse(Console.ReadLine(), out newmaxSpeed))
            {
                Console.WriteLine("Invalid max speed!");
                return;
            }

            Console.Write("What is the max horsepower? ");
            int newhorsepower;
            if (!int.TryParse(Console.ReadLine(), out newhorsepower))
            {
                Console.WriteLine("Invalid max horsepower!");
                return;
            }

            SportsCar car = new SportsCar(newmaxSpeed, newhorsepower);
            bool sportCar = car.IsSportsCar();
            if (sportCar)
            {
                Console.WriteLine("This is Definitely a Sports Car");
            }
            else
            {
                Console.WriteLine("This is NOT a Sports Car");
            }
        }
    }
}
  • Removed underscores from variable names. I personally don't like them, but that's up to you (or your team standards).
  • Changed name of class to 'SportsCar' to meet .NET naming guidelines (classes should be initial caps in a form known as PascalCasing).
  • The MinimumRequired variables are constant, so they are declared as const.
  • Since the maxSpeed and horsepower should be invariant for a Car, I made them readonly and settable via the constructor.
  • Consequently, they are now private but accessible via getter properties.
  • The IsSportsCar method is simplified since the evaluation is already a boolean construct (i.e. no need for a return true and return false as it evaluates as such).
  • Made the class sealed so no classes can inherit from it. By default, I seal all my classes and only remove the sealant if design dictates it.
  • The Program class is now static since it only contains a single static method.
  • Employed use of int.TryParse() in place of int.Parse() in order to allow program execution be smoother if an invalid input is typed.
  • Removed extraneous 'using' directives.
  • Fixed spelling of "Definitely".

I'd also add liberal amounts of commenting. .NET has a really good documentation system which allows for "XML Comments" at the class, variable, property and method level which would look something like this:

    /// <summary>
    /// Determines whether this particular car meets the criteria of being a sports car.  Its maximum speed must
    /// be at least MinimumRequiredSpeed AND the horsepower must be at least MinimumRequiredHorsepower.
    /// </summary>
    /// <returns>
    /// <c>true</c> if this car is a sports car; otherwise, <c>false</c>.
    /// </returns>
    public bool IsSportsCar()
    {
        return (this.maxSpeed >= MinimumRequiredSpeed) && (this.horsepower >= MinimumRequiredHorsepower);
    }

That's just an example, do not underestimate the power of in-code commenting as well.

One last thing I thought of. There's an object oriented principle known as SRP, or Single Responsibility Principle. It means that each class you design should have one and only one purpose which is rather limited in scope. The same thing can be said for methods themselves. You never know when you might need to refactor logic out of a method to use elsewhere. So even though this is a trivial example, you might want to look into rewriting IsSportsCar() into three separate methods:

    public bool IsSportsCar()
    {
        return this.MeetsMinimumSpeedRequirements() && this.MeetsMinimumHorsepowerRequirements();
    }

    private bool MeetsMinimumSpeedRequirements()
    {
        return this.maxSpeed >= MinimumRequiredSpeed;
    }

    private bool MeetsMinimumHorsepowerRequirements()
    {
        return this.horsepower >= MinimumRequiredHorsepower;
    }

Now, if your SportsCar ever needs to do some checks against only speed or horsepower, you have it logically separated out.

Best of luck!

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

You code looks reasonable, but I would have couple of relatively minor suggestions:

  • Did you consider what happens when user enters an invalid number? I.e. a string that contains letters or even a string that can be interpreted as a number but is not in valid range (e.g. is negative or too large to be applicable to any real-life car)? So, you might want to look at handling potential exceptions from int.Parse and also to define some sort of acceptable range of values. If you really want to impress your teacher, make this range configurable (using Properties.Settings mechanism). The maxSpeed method is where you would check the range validity.
  • Make _maxSpeed (and _horsepower) private.
  • The naming of maxSpeed (and horsepower) is slightly misleading. On the first glance, it looked like it would return the max speed, not set it. Only after visually "parsing" the method signature and/or the way it is called, it becomes clear that this is actually a setter - the name alone dos not suggest its purpose. So either change the name to setMaxSpeed, or use a write-only property (and a "real" setter).
  • Rename sportsCar to just car, since, after all, IsSportsCar is not guaranteed to always return true.
  • Not all of your using directives are necessary.
\$\endgroup\$
  • \$\begingroup\$ -1 (or it would be, if I had the rep): while your answer is useful (though not complete), I find it a very bad suggestion to start the property name with a verb, since it is not a method; also, in C# usually class properties and methods start with caps, and local variables start with low caps. \$\endgroup\$ – ANeves Oct 18 '11 at 7:32
  • 2
    \$\begingroup\$ @ANeves They aren't properties, they are setter methods. \$\endgroup\$ – Kirk Broadhurst Oct 21 '11 at 2:47
  • \$\begingroup\$ @KirkBroadhurst, I don't understand how "setter methods" can mean anything other than properties. Can you explain? \$\endgroup\$ – ANeves Oct 21 '11 at 4:15
  • \$\begingroup\$ @ANeves Well, the second sentence of your link explains it somewhat -"Properties can be used as though they are public data members". It is were a property, you would be able to assign directly to it like maxSpeed = value, rather than maxSpeed(value). See Using Properties - "To the user of an object, a property appears to be a field, accessing the property requires exactly the same syntax." \$\endgroup\$ – Kirk Broadhurst Oct 21 '11 at 4:27
  • \$\begingroup\$ @KirkBroadhurst ooooh, sorry, I just noticed I've been assuming that OP was using properties!... while OP was actually using "setter methods" as you said. But then, my comment would then be something like: -1: while your answer is useful (though not complete), I find it a very bad suggestion to use "setter methods" instead of properties; also, in C# usually class properties and methods are in CamelCase, and local variables in lowCamelCase. \$\endgroup\$ – ANeves Oct 21 '11 at 5:58
1
\$\begingroup\$
SportsCar mySportsCar = New Sportscar();
If (mySportsCar.IsSportsCar())
{
    //...

Well... Doesn't that look odd? I wouldn't expect a SportsCar to be anything but a sportscar. This isn't a SportsCar class. It's a Car class with a IsSportsCar() method. You could either rename the class, or create a constructor that validates the horsepower and speed upon creation.

I'm going to add a slight modification to Jesse C. Slicer's answer. If either the speed or horsepower are not sufficient, we'll throw an exception.

        private const int MinimumRequiredSpeed = 150;
        private const int MinimumRequiredHorsepower = 250;
        private readonly int maxSpeed;
        private readonly int horsepower;

        public SportsCar(int maxSpeed, int horsepower)
        {
            If (maxSpeed < MinimumRequiredSpeed)
            {
                throw New ArgumentException("Too slow to be a sportscar");
            }

            If (horsepower < MinimumRequiredHorsepower)
            {
                throw New ArgumentException("Not enough horsepower to be a sportscar");
            }

            this.maxSpeed = maxSpeed;
            this.horsepower = horsepower;
        }

That's just an example. In reality, I would report back what the minimum speed/horsepower is.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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