Suggestions needed after modification of Simulation of an Ocean

After suggestions given in this question, modifications to the code has been done.

Only thing I could not do is to decide, where to place the starveTime property (in Shark class or somewhere in a common place) because starveTime is not specific to Shark, it is common to any eating creature (other fish), currently Shark persist eating creature.

Please review from OOPS aspect/memory usage/... aspect.

Here is the modified code.

/* point.java */

/**
 * The Point class defines a location (x,y) in the Ocean.
 * 
 * @author mohet01
 *
 */
public class Point {
    /**
     * Here top-left of Windows screen is considered as origin.
     * x is an x-coordinate of a location in an ocean 
     * y is an y-coordinate of a location in an Ocean
     */
    private int x;
    private int y;

    /**
     * Constructor creates a Point object below co-ordinates
     * @param x
     *          is an x-coordinate of a Critter location in an Ocean
     * @param y
     *          is an y-coordinate of a Critter location in an Ocean
     */
    public Point(int x, int y){
        this.x = x;
        this.y = y;
    }

    /**
     * This method returns the x-coordinate of Critter in an ocean
     * @return
     *          x-coordinate of a Critter location in an Ocean. 
     */
    public int getX(){
        return this.x;
    }

    /**
     * This method returns the y-coordinate of Critter in an ocean
     * @return
     *          y-coordinate of a Critter location in an Ocean. 
     */
    public int getY(){
        return this.y;
    }

}

```
/ Critter.java /

/**
* The abstract class Critter defines a base class for anything(which can be empty)
* that can exist at a specific location in the ocean.
* @author mohet01
*
*/
public abstract class Critter {

/**
* Below data member defines a location of a Critter in an Ocean
*/

Point location;

public Critter(int x, int y){
location = new Point(x,y);
}

public Point g

Critter

Your Point class is, as @RoToRa pointed out, underused. This constructor:

Point location;

public Critter(int x, int y){
    location = new Point(x,y);
}

Could be simplified to this:

Point _location;

public Critter(Point location){
    _location = location;
}

Also in this snippet:

/**
 * This method computes the new value of location(which can be EMPTY) property of Critter.
 * No operation is performed as this is a base class.
 */
public abstract Critter update(Ocean currentTimeStepSea);

The comment "no operation is performed as this is a base class" is redundant, because the method is abstract - don't bother writing comments that reword what the code already says.

I'll skip the implementing classes and jump dive straight into the Ocean.

Ocean

You've replaced the lightweight integers you had in your array with objects. That's great, but you still have an array with tons of objects you don't need to care about - the Empty class doesn't need to exist. Let's see why.

• An ocean has critters and updates periodically.

• A critter has a location and updates periodically. To do so it needs to know about other critters in the sea, so the Update method takes in a Iterable. A critter can also be "dead" and need to be removed from the ocean.

Update

Your current implementation starts like this:

Ocean nextTimeStepSea = new Ocean(width, height);

So at every time step, you're re-creating the entire ocean?! What is it exactly that needs to happen in the Ocean's update method? Every critter must be updated. The easiest is to iterate the _critters list and call each instance's update method, passing in the _critters list as the Iterable parameter.

After all critters have updated, the ocean's update method should spawn the new critters (add them to _critters) and destroy (remove them from _critters) the dead ones; you'll want to do that after a first iteration over all critters, for sanity's sake.

Note that the Ocean class doesn't need to expose an addCritter or removeCritter method to do that.

To make your code more object-oriented, you'll need to think in terms of objects, not just in terms of solving a problem. Forget about the oceanMatrix[][] - the ocean doesn't even need to have a size. If you want to have a maximum size, it's not really the ocean's size you're defining, but the boundaries of the virtual "box" a critter is allowed to move in. Thus, I'd probably define these as private static final Point maxMove fields (or whatever the Java equivalent of C# private static readonly Point maxMove is) in the Critter base class. What you don't want to be doing here, is declaring that Point on the Ocean class, and tightly couple critters with the Ocean class by merely referring to it: it's important that critters don't even know an ocean exists.

To facilitate the implementation of your paint method to render the ocean and its critters on-screen, you could implement a Critter getContent(Point) method on the Ocean class that returns either null or the Critter instance that populates the specified coordinate; if the method returns null you draw nothing, if it returns a Critter that's alive (in theory the ocean's update method will have removed dead critters so that wouldn't need to be checked), you can draw a fish if it's a Fish, and you can draw a shark if it's a Shark - that's one simple, naive way of going about it, but it's a start.

The bottom line, is that Ocean must stop being represented as an array of X-Y coordinates with a "content", and start being a composition of objects with their own concerns. I'm sure the Utility class will become useless when everything is in its place.

As for where the starveTime should go... how about deriving a PredatorCritter class from Critter, putting starveTime in there, and deriving Shark from PredatorCritter?

StackExchange Code Review Q#43921, answer score: 8