What is the Java standard for object cohesion? Does too much information in an object mean bad design? - see example

Question

I'm creating a project which models an airport landing system. I have a plane object which stores all the information I need to sort the plane into a queue and store in a database. All the vital information is included in the object but I have also included the co-ordinates for each plane. My issue is that it may not be considered cohesive because each plane does a lot of different things.

I just want to know if this is considered bad design or is there a better way to do this?

Also, what is the "rule" for cohesion inside of objects? is there a specific design pattern that can maybe deal with this?

public class Plane extends Aircraft {
/*
 * Flight status should only take one of these enum values
 */
private static enum Status {
    REGISTERED, IN_QUEUE, LANDING, LANDED
};

// Set aircraft status to REGISTERED when created
private Status status = Status.REGISTERED;

private double fuelLevelPercentage;
private int passengerCount;
private int aircraftNumber;
private String airlineCompany;
private String departureAirport;

// This is used by the constructor to assign a random city to each new Aircraft
private final String[] cities = { "Rome", "Berlin", "Heathrow",
        "Edinburgh", "Cardiff", "Dublin", "Stansted" };
// Used to set airline companies
private final String[] airLineCompanies =  { "Easyjet", "Ryanair", 
        "British Airways","Flybe","Air Lingus", "Virgin" }; 


// Random number generator used by the constructor
private Random rand;

// Thread for this instance of Aircraft
private Thread aircraftThread;

// Radius of path when flying in circle (km?)
private final double FLIGHT_RADIUS = 10;
// Time taken to complete one complete loop (ms)
private final double FLIGHT_PERIOD = 120000;
// Angular frequency omega (rad/s)
private double OMEGA = 2 * Math.PI / FLIGHT_PERIOD;
// Time taken between being directed to land, and landing (ms)
private int TIME_TAKEN_TO_LAND = 30000;

// Time take to use one percent of fuel (ms)
private double time_taken_to_use_one_percent_of_fuel = 30000;

// variable to keep track of time since instantiated (ms)
private int time = 0;
// The aircraft Thread sleeps for TIME_STEP between updating
private final int TIME_STEP = 20;

private int time_when_called_to_land;

private int hour_of_arrival;
private int minute_of_arrival;

/*
 *  Set coordinates at time zero
 */
private double x_coord = 0;
private double y_coord = FLIGHT_RADIUS;
private double altitude = 1000;

/*
 *  Used to calculate path to airport
 */
private double x_coord_when_called;
private double y_coord_when_called;
private double altitude_when_called;

Calendar calendar = Calendar.getInstance();

/**
 * This constructor sets the following fields to random values Dummy Data -
 * should have a better way to do this
 */
public Plane() {
    rand = new Random();

    this.fuelLevelPercentage = rand.nextInt(100);
    this.departureAirport = cities[rand.nextInt(cities.length)];
    this.passengerCount = rand.nextInt(500);
    this.aircraftNumber = rand.nextInt(50000000);
    this.airlineCompany = airLineCompanies[rand.nextInt(airLineCompanies.length)];
}

/**
 * this fly method will call on a different method depending on the status
 * of the Aircraft
 */
public void fly() {
    if (status == Status.REGISTERED) {
        useFuel();
    } else if (status == Status.IN_QUEUE) {
        flyInCircle();
        useFuel();
    } else if (status == Status.LANDING) {
        flyToAirport();
        useFuel();
    } else if (status == Status.LANDED) {

    }
}

public void flyInCircle() {
    x_coord = FLIGHT_RADIUS * (Math.cos(OMEGA * (time)));
    y_coord = FLIGHT_RADIUS * (Math.sin(OMEGA * (time)));
}

public void flyToAirport() {
    if (!(x_coord < 1 && x_coord > -1 && y_coord < 1 && y_coord > -1
            && altitude < 1 && altitude > -1)) {
        x_coord -= x_coord_when_called * TIME_STEP / TIME_TAKEN_TO_LAND;
        y_coord -= y_coord_when_called * TIME_STEP / TIME_TAKEN_TO_LAND;
        altitude -= altitude_when_called * TIME_STEP / TIME_TAKEN_TO_LAND;
    } else {
        System.out.println("Aircraft landed");
        status = Status.LANDED;
        hour_of_arrival = calendar.get(Calendar.HOUR_OF_DAY);
        minute_of_arrival = calendar.get(Calendar.MINUTE);
    }

}

/**
 * This method changes the flight status to IN_QUEUE - simulates telling the
 * plane to join queue
 */
public void directToJoinQueue() {
    setFlightStatus(Status.IN_QUEUE);
}

/**
 * This method changes the flight status to LANDING - simulates telling the
 * plane to land
 */
public void directToflyToAirport() {
    setFlightStatus(Status.LANDING);
    time_when_called_to_land = time;
    x_coord_when_called = x_coord;
    y_coord_when_called = y_coord;
    altitude_when_called = altitude;
}

/**
 * This method reduces fuel level according to fuel usage
 */
private void useFuel() {
    if (this.fuelLevelPercentage - TIME_STEP
            / time_taken_to_use_one_percent_of_fuel > 0) {
        this.fuelLevelPercentage -= TIME_STEP
                / time_taken_to_use_one_percent_of_fuel;
    } else {
        this.fuelLevelPercentage = 0;
    }
}

/**
 * this method sets the flight status
 */
private void setFlightStatus(Status status) {
    this.status = status;
}

public double getfuelLevelPercentage() {
    return fuelLevelPercentage;
}

public int getPassengerCount() {
    return passengerCount;
}

public void setPassengerCount(int passengerCount) {
    this.passengerCount = passengerCount;
}

public int getAircraftNumber() {
    return aircraftNumber;
}

public String getDepartureAirport() {
    return departureAirport;
}

public void stop() {
    ;
}

public String getAirlineCompany() {
    return airlineCompany;
}

public void setAirlineCompany(String airlineCompany) {
    this.airlineCompany = airlineCompany;
}

@Override
public String toString() {
    if (status == Status.LANDED) {
        return String
                .format("Flight %-8d | Fuel %-4.1f | Passengers %-3d | From %-10s | %-8s at %d:%d ",
                        aircraftNumber, fuelLevelPercentage,
                        passengerCount, departureAirport, status,
                        hour_of_arrival, minute_of_arrival);
    } else {
        return String
                .format("Flight %-8d | Fuel %-4.1f | Passengers %-3d | From %-10s | %-8s | Coords (%-3.2f,%-3.2f) | Altitude %-4.2f",
                        aircraftNumber, fuelLevelPercentage,
                        passengerCount, departureAirport, status, x_coord,
                        y_coord, altitude);
    }

}

public void start() {
    aircraftThread = new Thread(this, this.getClass().getName());
    aircraftThread.start();
}

@Override
public void run() {

    try {

        while (true) {
            calendar = Calendar.getInstance();
            fly();
            Thread.sleep(TIME_STEP);
            time += TIME_STEP;

        }

        // System.out.println("aircraft number "+aircraftNumber+" safely landed");

    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

}
}

Answer 1

Cohesion is a difficult concept. Despite the other answer's flippant responses, the true answer depends very much on what your system does and how it works. For example lets examine the queue mechanism. In your system, does a plane respond to commands differently when in a queue? If so, then the fact that it is in a queue should be integral to the plane. Does it respond differently when in different queues? If so then the queue itself should be integral to the plane. If, however, it's the airport that responds differently because the plane is in a queue, then the airport should control the queue and the plane should know nothing about it -- it should simply be given a flight path by the airport (or by the control tower at the airport, depending on the resolution of your model).

Cohesion isn't your only problem here. Encapsulation is also a big issue. You are letting other objects have access to your internal state. To model this in a fully OO way, you should consider using the CQRS pattern. If you also consider DDD (Domain Driven Design) techniques, and start by identifying your bounded contexts and aggregate routes, you'll be more likely to derive a correct design.

Answer 2

There's no "standard" for Java or any other language.

I have a "plane" object which stores all the information I need to sort the plane into a queue and pass to a database. All the vital information is included in the object but I have also included the co-ordinates for each plane.

I think your Plane model object is doing too much.

I don't see why it should know whether or not it's in a queue. I'd have a separate object that owns the queue know the rules.

Is queue an in-memory collection or a message queue? Does it matter to you?

Model objects persisting themselves is a point of debate. I think it's easier to separate persistence into a separate data access object so it's easier to test.

Your model might look like this:

package model;

public class Plane {

   private int id;
   public void save() { 
      // persist the state of this
      // INSERT INTO PLANE(id) VALUES(?)
   }
}

I'd have a DAO interface in a separate package:

package persistence;

public interface PlaneDAO {
    void save(Plane p);
}

Answer 3

Cohesion can be defined as the degree to which the elements of a module belong together.

Visualizing it helps. Imagine the attributes of a class and the methods. If your class is cohesive, it means the methods are going to use many of the attributes, and conversely, the attributes will be used by many of the methods. This is the "sticking together" notion of cohesion. I like the following visualization that comes from NDepend's placemat:

As others pointed out, the methods that direct the plane (e.g., directToX) are possibly outside of the "theme" of what is a Plane, but they're not flagrantly wrong. Those elements (responsibilities) might be better in another class, say, AirTrafficController. In reality, planes don't decide much how they fly. Their pilots must follow instructions from the ground.

I'd argue that the Thread stuff (start, run) is definitely outside the theme of a Plane. Those methods hardly use anything that are part of a Plane (they are distractions from its theme). You could use an anonymous inner class to handle the processing in a thread from the main and your Plane would be even more reusable (and cohesive).

A cohesive object gets to the essence of the thing it models. This means it could more likely be re-used easily in another application (or even another OO language). Anything that starts to creep outside the true theme of your concept will likely make it harder to re-use the concept in another application. The "distractions" don't make sense anymore in another application.

If you're developing a Kamikaze project (one where you just want to make it work and don't care about re-use), it's perfectly OK to forget about cohesion (and other design elements). Design choices are trade-offs. You could refactor your Plane class to make it more cohesive, but if you never reuse it in another application, you've perhaps wasted your time. On the other hand, design is a learning process; even if you over-design something for one application, you maybe learned something for the next.

Finally, all design aspects are difficult to quantify and therefore there are few standards. Some companies have been known to set (arbitrary) standards for metrics such as LCOM in their development processes. I've read of team standards that say if a class has bad value for LCOM, then it must be refactored until its value goes low enough (its cohesion is stronger). Unfortunately, LCOM can be a bad measure of cohesion (especially in classes that have lots of get/set methods).

Answer 4

There is no java standard regarding object cohesion. (I don't repeat the advices of duffymo, I agree with all of them).

One thing to keep in mind when you elaborate an object model mapping the real world is to try to have one class mapping one concept of the real-world.

As an illustration, in your sample code you have at least 2 distincts concepts : Plane and Flight and so you can split them into 2 separate classes with a one-to-many relationship between them.