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);
}