Car

[Dietrich_1]

So I am having trouble with the last bit of the car class, I have to update the number of gallons in the car based on the number of miles driven...

public class Car
{    
    private double mpg;
    private double mileage;
    private double tankCapacity;
    private double gasInTank;

    /**
    Constructs a car.
    @param miles miles per gallon that a car gets
    @param milesOnCar total mileage on car
    @param gasTankHolds amount of gas the tank holds
    @param gasTankHas gas in tank now
     */
    public Car(double miles, double milesOnCar, 
    double gasTankHolds, double gasTankHas)
    {
        mpg = miles;
        mileage = milesOnCar;
        tankCapacity = gasTankHolds;
        gasInTank = gasTankHas;
    }

    /**
    Returns the current mileage on the car.
    @return mileage on car
     */
    public double getMileage()
    {
        return mileage;
    }

    /**
    Returns the amount of gas in the gas tank.
    @return amount of gas in tank
     */
    public double getGasInTank()
    {
        return gasInTank;
    }

    /**
    Returns the gas needed to drive miles (based on mpg)
    @param numMiles the number of miles to be driven
    @return gas needed to drive numMiles miles
     */
    public double gasNeeded(double numMiles)
    {
        double gasNeeded = numMiles/mpg ; 
        if(numMiles == 0)
        {
            return 0 ; 
        }
        else if(mpg == 0)
        {
            return 0 ;
        }
        else

            return gasNeeded ; 

    }

    /** 
    Returns true if the car has enough gas in the tank to
    drive numMiles, otherwise returns false.
    @param numMiles miles to be driven
    @return true if gas in tank is enough to drive numMiles
    miles; false otherwise.
     */
    public boolean enoughGas(double numMiles)
    {    
        double gasNeeded = numMiles/mpg ; 
        if(gasInTank == gasNeeded)
        {
            return true ; 
        }
        else
        {
            return false;  
        }
    }

    /**
    If tank is less than half full, fills tank and updates
    gasInTank.  Otherwise does nothing.
     */
    public void getGas()
    { double gas2 = 1.0/2.0 * tankCapacity ; 
        if(gasInTank <= gas2) 
        {
            gasInTank = tankCapacity ; 

        }
    }

    /**
    Updates mileage and gasInTank to reflect numMiles
    being driven.
    @param numMiles number of miles driven
     */
    public void drive(double numMiles)
    {
        double gasNeeded = numMiles/mpg ; 
        if(numMiles != 0 )
        { mileage = mileage + numMiles ; 
        }

}
}

[rproffitt]

Just a thought. Isn't this a little wordy? For example the if statement
if(gasInTank == gasNeeded)
and such. Couldn't that be a one liner like:
return (gasInTank == gasNeeded);

I could be wrong but I am working an android app that went to China, India and back home that is full of oddities and long passages that go on and on that could be stated simply in one line of code.

[stultuske]

Your code is quite a bit ... goofy. Take a close look at what you have written, and see what you can improve where.

An example:

public void getGas()
    { double gas2 = 1.0/2.0 * tankCapacity ; 
        if(gasInTank <= gas2) 
        {
            gasInTank = tankCapacity ; 
        }
    }

why is this called getGas? getters are supposed to return a value, so a method called getGas would have to return the amount of gas currently there (in order to make sense to anybody that's using your code)

a method is in fact a member that should perform one single task. your method performs two: 1. checking whether you need more gass and 2. actually filling it up.
you can split this up by:

public boolean needToFillTank() {
  return gasInTank <= (1.0/2.0 * tankCapacity);
  }

and

  public void fillTank() {
    this.gasInTank = this.tankCapacity;
  }

Part of this can actually be avoided. If you think about it, you can keep a boolean that keeps track of whether you need to refill or not. Just like a real car, the low-on-gas light starts blinking when you are low on gas, not when you ask it whether you are low on gas.

To do this, add

private boolean lowOnGas;

private void setLowOnGas() {
  this.lowOnGas = gasInTank <= (1.0/2.0 * tankCapacity);
}

and call this setLowOnGas method each time you update the value of gasInTank (starting from the constructor)

But, and here is what's missing: you never update the value of gasInTank in your code, so it will always keep the value you assign to it in the constructor.

Now, the next method, is an Exception just waiting to happen:

public void drive(double numMiles)
    {
        double gasNeeded = numMiles/mpg ; 
        if(numMiles != 0 )
        { mileage = mileage + numMiles ; 
        }
}

Change it to:

public void drive(double numMiles)
    {
        if(numMiles != 0 )
        { 
            double gasNeeded = numMiles/mpg ; 
            mileage = mileage + numMiles ; 
        }
}

just to avoid a division by 0 to happen. Or, better yet, since you don't actually use the value of gasNeeded, remove the line alltogether.

public boolean enoughGas(double numMiles)
{    
    double gasNeeded = numMiles/mpg ; 
    if(gasInTank == gasNeeded)
    {
        return true ; 
    }
    else
    {
        return false;  
    }
}

Again, you risk an Exception when you pass 0 as parameter.

public boolean enoughGas(double numMiles)
{    
    if ( numMiles == 0 ){
      return true;
    }
    double gasNeeded = numMiles/mpg ; 
    return gasInTank == gasNeeded;
}

It can be even shorter, but just to keep it clear, this 'll do

public double gasNeeded(double numMiles)
{
    double gasNeeded = numMiles/mpg ; 
    if(numMiles == 0)
    {
        return 0 ; 
    }
    else if(mpg == 0)
    {
        return 0 ;
    }
    else
        return gasNeeded ; 
}

You really need to keep an eye on code smells like this. This is the third method with the same issue.

 public double gasNeeded(double numMiles)
    {
        if  ( numMiles == 0 || mpg == 0 ){
          return 0;
        }
        // There's no need for an else statement. in each case this line shouldn't run, the method would already have finished on the return 0; line
        // There's also no need to save numMiles/mpg in a separate variable, since you're not actually doing anything with the value
        return numMiles/mpg ; 
    }

[JamesCherrill]

A couple of notes re the above:

The discussion of divide by zero seems to be looking at the wrong variable. It's not the miles that are the problem, its the mpg that cannot be zero.

As rproffitt points out, using an if/else to decode a boolean is highly redundant, so

    if(gasInTank == gasNeeded)
    {
        return true ; 
    }
    else
    {
        return false;  
    }

can just be

return gasInTank == gasNeeded;

ps Opinions vary about how to bracket code, but the norm for Java, as used in all the code for the API as well as all Oracle's samples, documentation and tutorials, is

        if(gasInTank == gasNeeded) {
            return true ; 
        } else {
            return false;  
        }