How should classes be structured internally as regards methods and their use of instance variables?

Question

I keep having problems deciding how I should create my classes internally. Initially I would have an external class handle all the variable management:

String destination = car.setDestination("San Fransisco");
int totalGas = car.getAmountOfGas();
int requiredGas = car.gasRequiredForDestination(destination);
boolean enough = car.hasEnoughGas(totalGas, destination);
if (enough) 
   car.travelToDestination()

But it seemed really strange to me that another class should be doing all the work for the car class's data, since the car should be able to do most of the work itself. So to fix that I thought... "hmmm let me just stick all this in the class where it seems like it should go". I figured by doing this I could avoid having to pass so much data back and forth between methods. So then I got this:

Class Car {
  String location = "home";
  String destination;
  int totalGas = 0;
  int requiredGas = 0;
  boolean enoughGas = false;

  public Car (String destination, int totalGas) {
    this.destination = destination;
    this.totalGas = totalGas;
  }
  public boolean travelToDestination() {
     getGasRequiredForDestination();
     hasEnoughGas();
     if (enoughGas == true)
        location = destination;
  }

So the problem I encountered here is that now yes I don't have to pass the data around and things look real clean, but now I am dependent upon each function to return the value to the instance variable. Which in itself isn't terrible, but this just seems very wrong to me. At the same time I think to myself "well I doesn't make sense to pass all this data from one place to another when it seems like I should just manipulate the instance variables. On the other hand my programs end up having 6 lines in a row of:

myMethod {
  doThis()
  doThat()
  checkThis()
  checkTheOtherThing()
}

I never really see things done this way in real life so I'm trying to figure basically A) if this is wrong B) if so when should we stick information in instance variables rather than pass it all around. Object Orientation allows us to do this but I don't know if it's a good thing. C) Are there any OO principles involved in doing or not doing things this way? Maybe something I'm violating that I'm not aware of?

I've been programming OO for a long time but I tend to have issues like this on and off so I was hoping to sort it out. If there are any book recommendations that deal with the trickier side of OO I'd be interested in that too.

EDIT: I should have said right off that this is a made up example so there are things in the real world I probably would not do this way necessarily. But I needed some sort of example as the code I had was too complicated.

Answer 1

Well this question can be explained in detail by wiser heads than me. But here is my take:

I tend to try to define classes as some data, and set of operations that need to be performed on them (following class inheritance hierarchy of course). So your approach of encapsulating operations on Car class is right because in this case you would just do

Car.travelToDestination()

and it would be fine.

myMethod {
  doThis()
  doThat()
  checkThis()
  checkTheOtherThing()
}

is not wrong in the sense that all your methods above are doing ONE logical operation each (I strongly recommend making methods do ONLY one logical operation) and they are being used correctly.

Regarding class data being passed around, it is generally considered a good design if you can encapsulate data and operations on it internally in one class, which seems to be the how you want to do in your example.

Regarding book recommendation, I found for myself Code Complete 2 has an excellent chapter on class design (chapter 6 called Working Classes, I think) that should address doubts like this. I find myself referring to it quite a bit. Either way I think that book should be required reading for all programmers.

Answer 2

I think mabe your problem is the idea that there should be "one class where it should go". I don't pretend to understand the domain you are talking about, but some obvious classes relevant to the problem seem to be:

• car
• map
• trip planner
• location
• waypoint

I don't normally ask a car if it can get from one location to another - I might ask it how far can it go on it's current gas tank load. I wouldn't expect it to know how to get to SF or the mileage, I get some of that from the map, using waypoints. And all of this should be being coordinated by the trip planner, which comes up with the final answer.

All of these classes will have their own specialised member data and will interact using specific member functions. Some of these functions will take instances of other classes as parameters.

Answer 3

Indeed you shouldn't use instance variables to pass data to methods. Why not use function results within one method?

public boolean travelToDestination(string destination) {
   int requiredGas = getGasRequiredForDestination(destination);
   boolean enoughGas = hasEnoughGas(requiredGas);
   if (enoughGas) {
      location = destination;
      totalgas -= requiredgas;
   }
}

This makes it clear to the reader of your code how getGasRequiredForDestination() and hasEnoughGas() work, without having to look for side effects. It's called principle of least astonishment.

Instance variables should be used to store the state of an object after the control flow has left the class methods, in this case location and totalgas after control returned from travelToDestination(). If the control should return earlier, e.g. when the car is traveling to its destination over multiple steps, then you need to store additional information in field variables.

Answer 4

Try reasoning a bit more abstractly: in as much as an instance of your class is modeling a real-world entity (a good thing, when you can do that conveniently), instance variables should be all that you need to represent the state of that thing -- not artefacts such as temporary results of computations, which don't correspond to any real-world "state".

So, for example, consider your class:

Class Car {
  String location = "home";
  String destination;
  int totalGas = 0;
  int requiredGas = 0;
  boolean enoughGas = false;

and critique it based on the test "is this instance variable actually part of the state, or not?".

By this criterion, location and totalGas seem fine -- a real-world car does indeed have a location, and some amount of gas in it, as part of its real-world state. The others are more dubious. destination would be fine if you were representing the car at various spots during a trip, or a leg of the trip -- at any given time there would be a present spot, and a destination towards which the car is traveling. But judging from your code that's not what you're doing: the destination instantly becomes the location if gas is sufficient, so you're using a simplified model of reality where the car is only represented as being in specific places rather than in route between them (which btw is perfectly fine: any abstraction is, inevitably and usefully, a simplification of reality, and if for your application's purposes you can abstract away the "traveling between places" state, by all means go for it). The same applies even more strongly to the variables about required and enough gas -- not natural parts of the object's state.

So make those local variables, arguments, and return values, for the appropriate methods, i.e., change the traveling method to:

  public void travelToDestination(String destination) {
     int requiredGas = getGasRequiredForDestination(destination);
     bool enoughGas = hasEnoughGas(requiredGas);
     if (enoughGas) {
        totalGas -= requiredGas;
        location = destination;
     }
  }

So, some of the values needed for computation (precisely those that are part of the object's state) are in instance variables, other (the intermediate results of computations that are not actually part of object state) are local variables, arguments, return values.

This mixed approach is sounder and more promising than either your original one (with all those "getter" method calls, eep!-) or the one at the other extreme (where everything and its cousin was an instance variable, for mere computational convenience and quite apart from good modeling approaches).

Since instance variables and local variables therefore get mixed in most computations, many programming styles require them to have distinguishable names (some languages such as Python and Ruby make that mandatory -- the instance variable location for example would be spelled @location or self.location -- but I'm talking about styles for languages that do not force the issue, but still allow you to name that instance variable location_ with a trailing underscore, or m_location with an m_ prefix, and the like).

Answer 5

I would pull out a Trip object that contains the origin and destination, and can calculate the distance. I don't think it's the responsibility of a car to understand and operate on the details of a Trip. It is the responsibility of a car to know its mileage and how far it can travel, and to go places.

There's two directions you could go from here. One would be to give the responsibility of performing the trip to the Car, something like:

Trip trip = new Trip(origin, destination);
Car car = new Car();
car.fillTank();

if (car.canTakeTrip(trip))
    car.takeTrip(trip);

But this bugs me. It couples Car to Trip and results in two high-level calls to car, both of which operate on a Trip. This suggests you might want to have a Trip member in car and call car.setTrip(trip)...but now we know we're down a wrong path. This way leads to madness. Car is now not only coupled to Trip, but can own an object that has nothing to do with Car. You wouldn't ask "what Trip does that car have?" You'd ask "where is that car going?".

A car should do simple car things. Fill up with gas. Drive here. Drive there. It shouldn't know about such things as "Trips." This suggests that we invert the responsibility and give the job of taking the trip to the Trip itself. Maybe something like:

trip.setCar(car);
trip.travel();
System.out.println(trip.result); // might be "Not enough gas" or "Arrived! Wish you were here!"

And inside the call to trip.travel() we might see:

public void travel()
{
    if (this.car.canTravel(this.distance))
    {
         car.travel(this.destination);
    }
    else
    {
         this.result = "Not enough gas";
    }
}

But we've still coupled Trip to Car, so we might like to pull up a Transport interface that hides the implementation. It makes more sense to me that a Trip would own a Transport than having Car own a Trip. A Transport interface might be:

interface Transport
{
    public boolean canTravel(int distanceInMiles);
    public void travel(String destination); // seems like we need more than a String here, but you get the idea
}

Now Car would implement like

class Car implements Transport
{
    // implement canTravel and travel
}

And finally Trip might do something like:

class Trip
{
    public void travel(Transport transport)
    {
        if (transport.canTravel(this.distance))
        {
             transport.travel(this.destination);
        }
        else
        {
             this.result = "Not enough gas";
        }

    }
}

Car car = new Car();
car.fillUp();
Trip trip = new Trip(); // maybe set Transport on constructor?
trip.travel(car);

Or something like this. I generally try to have my objects to the simplest things that come natural to those objects. If they start obtaining knowledge that doesn't seem to flow naturally into a few (preferably one) responsibility, then I look for the missing Class that I need to define and start trying to invert responsibilities.

Answer 6

This is how I would do it:

Class Car {
    String location = "home";
    int totalGas = 0;

    public Car (int totalGas) {
        this.totalGas = totalGas;
    }

    public boolean travelToDestination(String destination) {
        int requiredGas = getGasRequiredForDestination(destination);
        if(totalGas >= requiredGas){
            location = destination;
            totalGas -= requiredGas;
            return true;
        } else {
            return false;
        }
    }
}

The idea is to have as little state as possible, and by "state" I mean variables outside the scope of the method (e.g. member variables, globals, etc.). I'm not sure if this principle has a specific name, but I've definitely run into similar principles in multiple places while reading. I can also attest to benefits of reducing state from personal experience.

Every extra member variable decreases maintainability and increases the opportunity for bugs to be introduced. Only make member variables if they can't be local variables.

Answer 7

An object should have three aspects; state, behaviour and identity. A key metric for an object is that it have high internal cohesion (and low external coupling).

There's absolutely nothing wrong with having your methods act on shared internal state.

I know you said that your example was made up to illustrate a point but the 'Car' looks more like a 'Journey' or 'Route'