Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

As part of my Java learning, I tried solving part I of problem description here. The only issue that I see now is that I could not close the button of Frame.

Please review and provide your comments on coding style/OOPS/.. aspect on below files ocean.java, simtext.java and simulation.java.

Ocean.java

/* Ocean.java */

/**
 * The Ocean class defines an object that models an ocean full of sharks and
 * fish. Descriptions of the methods you must implements appear below. They
 * include a constructor of the form
 * 
 * public Ocean(int i, int j, int starveTime);
 * 
 * that creates an empty ocean having width i and height j, in which sharks
 * starve after starveTime timesteps.
 * 
 * See the README file accompanying this project for additional details.
 * 
 * @author mohet01
 * 
 */

public class Ocean {

    /**
     * Do not rename these constants. WARNING: if you change the numbers, you
     * will need to recompile Test4.java. Failure to do so will give you a very
     * hard-to-find bug.
     */

    public final static int EMPTY = 1;
    public final static int SHARK = 2;
    public final static int FISH = 3;


    /**
     * Define any variables associated with an Ocean object here. These
     * variables MUST be private.
     * 
     */

    private final static int UNKNOWN = -1; // for unknown return type
    private int width;
    private int height;
    private int[][] oceanMatrix;
    private int[][] sharkHungerLevelMatrix; // need to think on optimization
    private int starveTime;



    /**
     * The following methods are required for Part I.
     * 
     */

    /**
     * Ocean() is a constructor that creates an empty ocean having width i and
     * height j, in which sharks starve after starveTime timesteps.
     * 
     * @param i
     *            is the width of the ocean.
     * @param j
     *            is the height of the ocean.
     * @param starveTime
     *            is the number of timeSteps sharks survive without food.
     */

    public Ocean(int i, int j, int starveTime) {
        this.width = i;
        this.height = j;
        this.oceanMatrix = new int[j][i];
        this.sharkHungerLevelMatrix = new int[j][i];
        this.starveTime = starveTime;
        for (int row = 0; row < j; row++) {
            for (int col = 0; col < i; col++) {
                oceanMatrix[row][col] = EMPTY;
            }
        }
        for (int row = 0; row < j; row++) {
            for (int col = 0; col < i; col++) {
                sharkHungerLevelMatrix[row][col] = EMPTY;
            }
        }
    }

    /**
     * width() returns the width of an ocean Object.
     * 
     * @return the width of the ocean.
     * 
     */

    public int width() {
        return this.width;
    }

    /**
     * height() returns the height of an Ocean object.
     * 
     * @return the height of the Ocean.
     */

    public int height() {
        return this.height;
    }

    /**
     * starveTime() returns the number of timesteps sharks survive without food.
     * 
     * @return the number of timesteps sharks survive without food.
     */

    public int starveTime() {
        return starveTime;
    }

    /**
     * addFish() places a fish in cell (x,y) if the cell is empty. If the cell
     * is already occupied, leave the cell as it is.
     * 
     * @param x
     *            is the x-coordinate of the cell to place a fish in.
     * @param y
     *            is the y-coordinate of the cell to place a fish in.
     */

    public void addFish(int x, int y) {
        if (oceanMatrix[x][y] == EMPTY) {
            oceanMatrix[x][y] = FISH;
        }
    }

    /**
     * addShark() (with two parameters) places a newborn shark in cell (x, y) if
     * the cell is empty. A "newborn" shark is equivalent to a shark that has
     * just eaten. If the cell is already occupied, leave the cell as it is.
     * 
     * @param x
     *            is the x-coordinate of the cell to place a shark in.
     * @param y
     *            is the y-coordinate of the cell to place a shark in.
     */
    public void addShark(int x, int y) {
        if (oceanMatrix[x][y] == EMPTY) {
            oceanMatrix[x][y] = SHARK;
        }
    }

    /**
     * cellContents() returns EMPTY is cell (x,y) is empty, FISH if it contains
     * a fish, and SHARK if it contains a shark.
     * 
     * @param x
     *            is the x-coordinate of the cell whose contents are queried.
     * @param y
     *            is the y-coordinate of the cell whose contents are queried.
     */
    public int cellContents(int x, int y) {
        return oceanMatrix[x][y];

    }

    /**
     * isFish() checks for the existence of fish in that cell.
     * @param x 
     *              is the x-coordinate of the cell whose contents are queried.
     * @param y 
     *              is the y-coordinate of the cell whose contents are queried.
     * @return the boolean value
     */
    private boolean isFish(int x, int y){
        return (this.oceanMatrix[x][y] == Ocean.FISH);
    }

    /**
     * isShark() checks for the existence of shark in that cell.
     * @param x 
     *              is the x-coordinate of the cell whose contents are queried.
     * @param y 
     *              is the y-coordinate of the cell whose contents are queried.
     * @return the boolean value
     */
    private boolean isShark(int x, int y){
        return (this.oceanMatrix[x][y] == Ocean.SHARK);
    }

    /**
     * isSharkStarving() checks the hunger level of shark, if reached to starveTime level
     * @param x
     *              is the x-coordinate of the cell whose contents are queried.
     * @param y 
     *              is the y-coordinate of the cell whose contents are queried.
     * @return the boolean value
     */
    private boolean isSharkStarving(int x, int y){
        return (this.sharkHungerLevelMatrix[x][y] == (this.starveTime+1));  
    }


    /**
     * checkFish() checks the existence of atleast one fish 
     * surrounding shark cell
     * @param x
     *          is the x-coordinate of the cell whose contents are queried.
     * @param y
     *          is the y-coordinate of the cell whose contents are queried.
     * @return returns true on atleast one fish exist otherwise false
     * 
     */
    private boolean checkFish(int x, int y){
        for(int i = x-1;i <= x+1; i++){
            for(int j = y-1; j <= y+1; j++){
                if(this.isFish(mod(i,this.height), mod(j,this.width))){     
                    return true;

                }
            }
        }
        return false;
    }

    /**
     * countShark() counts the number of sharks surrounding queried cell
     * @param x
     *          is the x-coordinate of the cell whose contents are queried.
     * @param y
     *          is the y-coordinate of the cell whose contents are queried.
     * @return returns number of sharks surrounding fish cell
     */

    private int countShark(int x, int y){
        int neighbourSharkCount = 0;
        for(int i = x-1;i <= x+1; i++){
            for(int j = y-1; j <= y+1; j++){
                if(this.isShark(mod(i,this.height), mod(j,this.width))){
                    neighbourSharkCount++;
                }
            } // end inner for loop
        }//end outer for loop
        return neighbourSharkCount;
    }

    /**
     * countFish() counts the number of fish surrounding queried cell
     * @param x
     *          is the x-coordinate of the cell whose contents are queried.
     * @param y
     *          is the y-coordinate of the cell whose contents are queried.
     * @return returns number of sharks surrounding queried cell
     */

    private int countFish(int x, int y){
        int neighbourFishCount = 0;
        for(int i = x-1;i <= x+1; i++){
            for(int j = y-1; j <= y+1; j++){
                if(this.isFish(mod(i,this.height), mod(j,this.width))){
                    neighbourFishCount++;
                }
            } // end inner for loop
        }//end outer for loop
        return neighbourFishCount;
    }


    /**
     * mod() performs the modulo operation using euclidean divison
     * 
     * @param n
     *            is the numerator
     * @param d
     *            is the denominator
     * @return the remainder
     */

    private int mod(int n, int d) {
        if (n >= 0)
            return n % d;
        else
            return d + ~(~n % d);
    }

    /**
     * timeStep() performs a simulation timestep as described in README.
     * 
     * @return an ocean representing the elapse of one timestep.
     */
    public Ocean timeStep() {

        Ocean sea = new Ocean(width, height, starveTime);

        for (int row = 0; row < this.height; row++) {
            for (int col = 0; col < this.width; col++) {

                        switch(this.oceanMatrix[row][col]){

                        case Ocean.SHARK: 
                            boolean gotTheFish = false;
                            //Check all the 8 neighbors of a Shark Cell for fish
                            if(this.checkFish(row,col)){
                                gotTheFish = true;
                            }

                            //Updating Shark Cell
                            if(gotTheFish){
                                /*
                                 * 1) If a cell contains a shark, and any of its neighbors is a fish, then the
                                 * shark eats during the time step, and it remains in the cell at the end of the
                                 * time step.  (We may have multiple sharks sharing the same fish.  This is fine;
                                 * they all get enough to eat.)
                                 */
                                sea.oceanMatrix[row][col] = Ocean.SHARK; // for next time step
                            }else{
                                /*
                                 * 2) If a cell contains a shark, and none of its neighbors is a fish, it gets
                                 * hungrier during the time step.  If this time step is the (starveTime + 1)th
                                 * time step the shark has gone through without eating, then the shark dies
                                 * (disappears).  Otherwise, it remains in the cell.
                                 */
                                this.sharkHungerLevelMatrix[row][col]++;
                                if(this.isSharkStarving(row,col)){
                                    this.oceanMatrix[row][col] = Ocean.EMPTY; // for this time step
                                    this.sharkHungerLevelMatrix[row][col] = Ocean.EMPTY; // for this time step
                                }

                                sea.sharkHungerLevelMatrix[row][col] = this.sharkHungerLevelMatrix[row][col]; // for next time step
                                sea.oceanMatrix[row][col] = this.oceanMatrix[row][col]; // for next time step
                            }


                            break;

                        case Ocean.FISH:
                            int neighbourSharkCount=0;

                            //Check all the 8 neighbors of a Fish cell to count for sharks
                            neighbourSharkCount=countShark(row,col);

                            //Updating fish cell for current & next  time step
                            if(neighbourSharkCount ==1){
                                /*
                                 * 4) If a cell contains a fish, and one of its neighbors is a shark, then the
                                 * fish is eaten by a shark, and therefore disappears.
                                 */
                                this.oceanMatrix[row][col] = Ocean.EMPTY; //fish disappears this time step
                            }
                            else if(neighbourSharkCount  > 1){
                                /*
                                 * 5) If a cell contains a fish, and two or more of its neighbors are sharks, then
                                 * a new shark is born in that cell. Sharks are well-fed at birth; _after_ they
                                 * are born, they can survive an additional starveTime time steps without eating.
                                 */
                                sea.oceanMatrix[row][col] = Ocean.SHARK; // new shark for next time step
                            }
                            else if(neighbourSharkCount  < 1){
                                /*
                                 * 3) If a cell contains a fish, and all of its neighbors are either empty or are
                                 * other fish, then the fish stays where it is.
                                 */
                                sea.oceanMatrix[row][col] = FISH; //for next time step
                            }
                            break; 

                        case Ocean.EMPTY:
                            int fishCount=0;
                            int sharkCount=0;

                            //Check all the 8 neighbors of an Empty cell to count sharks and Fish
                            fishCount = this.countFish(row,col);
                            sharkCount = this.countShark(row, col);

                            //Update Empty Cell for current & next time step.

                            /* (no need to handle this case)
                             * 6) If a cell is empty, and fewer than two of its neighbors are fish, then the
                             * cell remains empty.
                             */

                            if((fishCount >= 2) && (sharkCount <=1)){
                                /*
                                 * 7) If a cell is empty, at least two of its neighbors are fish, and at most one
                                 * of its neighbors is a shark, then a new fish is born in that cell.
                                 */
                                this.oceanMatrix[row][col] = FISH;// for current time step 
                                sea.oceanMatrix[row][col] = FISH; //for next time step
                            }else if((fishCount >= 2) && (sharkCount >= 2)){
                                /*
                                 * 8) If a cell is empty, at least two of its neighbors are fish, and at least two
                                 * of its neighbors are sharks, then a new shark is born in that cell. (The new
                                 * shark is well-fed at birth, even though it hasn’t eaten a fish yet.)
                                 */
                                sea.oceanMatrix[row][col] = Ocean.SHARK; // for next time step
                            }
                            break;
                        }
            }//end inner for loop
        }//end outer for loop
        return sea;
    }

    /**
     * The following method is required for Part II.
     * 
     * 
     */
    /**
     * addShark() (with three parameters) places a shark in cell (x, y) if the
     * cell is empty. The shark's hunger is represented by the third parameter.
     * If the cell is already occupied, leave the cell as it is, You will need
     * this method to help convert run-length encodings to Oceans.
     * 
     * @param x
     *            is the x-coordinate of the cell to place a shark in.
     * @param y
     *            is the y-coordinate of the cell to place a shark in.
     * @param feeding
     *            is an integer that indicates the shark's hunger. You may
     *            encode it any way you want; for instance, "feeding" may be the
     *            last timestep the shark was fed, or the amount of time that
     *            has passed since the shark was last fed, or the amount of time
     *            left before the shark will starve. It's upto you, but be
     *            consistent.
     */

    public void addShark(int x, int y, int feeding) {
        this.oceanMatrix[x][y] = Ocean.SHARK;
        this.sharkHungerLevelMatrix[x][y] = feeding;
    }

    /**
     * The following method is required for Part III.
     */

    /**
     * sharkFeeding() returns an integer that indicates the hunger of the shark
     * in cell (x, y), using the same "feeding" representation as the parameter
     * to addShark() described above. If cell (x, y) does not contain a shark,
     * then its return value is undefined--that is, anything you want. Normally,
     * this method should not be called if cell (x, y) does not contain a shark.
     * You will need this method to help convert Oceans to run-length encodings.
     * 
     * @param x
     *            is the x-coordinate of the cell whose contents are queried.
     * @param y
     *            is the y-coordinate of the cell whose contents are queried.
     * 
     */
    public int sharkFeeding(int x, int y) {
        if(this.isShark(x, y)){
            return this.sharkHungerLevelMatrix[x][y];
        }
        return Ocean.UNKNOWN;

    }
}

SimText.java

import java.util.Random;

/* SimText.java */

/* DO NOT CHANGE THIS FILE (except as noted). */
/* (You may wish to make temporary or insert println() statements   */
/* while testing your code. when you're finished testing and debugging, */
/* though, make sure your code works with the original version of this file. */

/**
 * The SimText class is a program that runs and animates a simulation of Sharks
 * and Fish.
 * 
 * The SimText program takes up to four parameters. The first two specify the
 * width and height of the ocean. The third parameter specifies the value of
 * starveTime. For example, if you run
 * 
 * java SimText 25 25 1
 * 
 * then SimText will animate a 25x25 ocean with a starveTime of 1. If you run
 * "java SimText" with no parameters, by default SimText will animate a 50x25
 * ocean with a starveTime of 3. With some choices of parameters, the ocean
 * quickly dies out; with others, it teems forever.
 * 
 * @author mohet01
 * 
 */

public class SimText {

    /**
     * Default parameters. (You may change these if you wish.)
     * 
     */
    private static int i = 50; // Default ocean width
    private static int j = 25; // Default ocean height
    private static int starveTime = 3; // Default shark starvation time

    /**
     * paint() prints an Ocean.
     */
    public static void paint(Ocean sea) {
        if (sea != null) {
            int width = sea.width();
            int height = sea.height();

            /* Draw the ocean */
            for (int x = 0; x < width + 2; x++) {
                System.out.print("-");
            }

            System.out.println();

            for (int row = 0; row < height; row++) {
                System.out.print("|");
                for (int col = 0; col < width; col++) {
                    int contents = sea.cellContents(row, col);
                    if (contents == Ocean.SHARK) {
                        System.out.print('S');
                    } else if (contents == Ocean.FISH) {
                        System.out.print('F');
                    } else {
                        System.out.print(' ');
                    }
                }
                System.out.println("|");
            }
            for (int x = 0; x < width + 2; x++) {
                System.out.print("-");
            }
            System.out.println();
        }// end if

    } // end paint

    /**
     * main() reads the parameters and performs the simulation and animation.
     * 
     * @param args
     * @throws InterruptedException
     */

    public static void main(String[] args) throws InterruptedException {
        Ocean sea;

        /**
         * Read the input parameters.
         */

        if (args.length > 0) {
            try {
                i = Integer.parseInt(args[0]);
            } catch (NumberFormatException e) {
                System.out
                        .println("First argument to SimText is not an number");
            }
        }

        if (args.length > 1) {
            try {
                j = Integer.parseInt(args[1]);
            } catch (NumberFormatException e) {
                System.out
                        .println("Second argument to SimText is not an number");
            }
        }

        if (args.length > 2) {
            try {
                starveTime = Integer.parseInt(args[2]);
            } catch (NumberFormatException e) {
                System.out
                        .println("Third argument to SimText is not an number");
            }
        }

        /**
         * Create the initial ocean.
         */

        sea = new Ocean(i, j, starveTime);

        /**
         * Visit each cell (in a roundabout order); randomly place a fish,
         * shark, or nothing in each. (21.5-15) / (21.5*2) is actually ~.15 so
         * this creates sharks 15% of the time. In otherwords random.nextInt()
         * will generate a number larger than 1500000000 15% of the time because
         * it is capable of generating 2147483647−1500000000 numbers larger than
         * 1500000000 out of a total of 2147483647−(−2147483648) numbers in
         * total.
         * 
         */

        Random random = new Random(0); // Create a "Random" object with seed 0
        int x = 0;
        int y = 0;
        for (int row = 0; row < j; row++) {
            x = (x + 78887) % j; // width - This will visit every x-coordinate
                                    // once
            if ((x & 8) == 0) {
                for (int col = 0; col < i; col++) {
                    y = (y + 78887) % i; // height - This will visit every
                                            // y-coordinate once
                    if ((y & 8) == 0) {
                        int r = random.nextInt(); // Between -2147483648 and
                                                    // 2147483647
                        if (r < 0) { // 50% of cells start with fish
                            sea.addFish(x, y); // x - width, y-height
                        } else if (r > 1500000000) { // ~15% of cells start with
                                                        // sharks
                            sea.addShark(x, y); // x - width, y-height
                        }
                    }
                }
            }
        }// end for loop

        /**
         * Perform timesteps forever.
         */

        while (true) { // Loop forever
            paint(sea);
            // For fun, you might wish to change the delay in the next line.
            Thread.sleep(1000); // Wait one second (1000 milliseconds)
            sea = sea.timeStep(); // Simulate a timestep
        }

    }// end main()
} // end Class SimText

Simulation.java

import java.awt.Canvas;
import java.awt.Color;
import java.awt.Frame;
import java.awt.Graphics;
import java.util.Random;

/* Simulation.java */

/* DO NOT CHANGE THIS FILE (except as noted). */

/* (You may wish to make temporary changes or insert println() statements)  */
/* while testing your code. When you're finished testing and debugging,   */
/* though, make sure your code works with the original version of this file */




/**
 * The Simulation class is a program that runs and animates a simulation of 
 * Sharks and Fish.
 * 
 * The Simulation program takes up to four parameters. The first two specify
 * the width and height of the ocean. The third parameter specifies the value 
 * of starveTIme. For example, if you run
 * 
 *          java Simulation 25 25 1
 *  
 * then Simulation will animate a 25x25 ocean with a starveTime of 1. If you 
 * run "java Simulation" with no parameters, by default Simulation will animate
 * a 50x25 ocean with a starveTime of 3. With some choices of parameters,
 * the ocean quickly dies out;  with others;, it teems forever.
 *  
 * @author mohet01
 *
 */
public class Simulation {

    /**
     * The constant cellSize determines the size of each cell on the screen
     * during animation.  (You may change this if you wish).
     */
    private static final int cellSize = 4;


    /**
     * Default parameters. (You may change this of you wish).
     */

    private static int  i = 50;                           //Default ocean width
    private static int  j = 25;                          //Default ocean height
    private static int starveTime = 3;          //Default shark starvation time


    /**
     * drawOcean() adds cell contents as part of graphics
     */

    private static void drawOcean(Graphics graphics, Ocean ocean){
        if(ocean != null){
            int width = ocean.width();
            int height = ocean.height();

            for(int row = 0; row < height; row++){
                for(int col = 0; col < width; col++){
                    int contents = ocean.cellContents(row, col);
                    if(contents == Ocean.SHARK){
                        graphics.setColor(Color.red);
                        graphics.fillRect(row*cellSize, col*cellSize, cellSize, cellSize);
                    }else if(contents == Ocean.FISH){
                        // Draw a green fish
                        graphics.setColor(Color.green);                
                        graphics.fillRect(row * cellSize, col * cellSize, cellSize, cellSize);
                    }else{
                        graphics.clearRect(row, col, cellSize, cellSize);
                    }
                }
            }
        }
    }


    /**
     * main() reads the parameters and performs the simulation and animation. 
     * @param args
     * @throws InterruptedException 
     */

    public static void main(String[] args) throws InterruptedException {
        Ocean sea;

        /**
         * Read the input parameters
         */

        if(args.length >0){
            try{
                i = Integer.parseInt(args[0]);
            }catch(NumberFormatException e){
                System.out.println("First argument to Simulation is not a number.");
            }
        }


        if(args.length > 1){
            try{
                j = Integer.parseInt(args[1]);
            }catch(NumberFormatException e){
                System.out.println("Second argument to Simulation is not a number");
            }
        }

        if(args.length > 2){
            try{
                starveTime = Integer.parseInt(args[2]);
            }catch(NumberFormatException e){
                System.out.println("Third argument to Simulation is not a number");
            }
        }



        /**
         * Create a window on your screen
         */

        Frame frame = new Frame("Sharks and Fish");
        frame.setSize(i*cellSize + 10, j*cellSize + 30);
        frame.setVisible(true);

        /**
         * Create a "Canvas" we can draw upon; attach it to the window 
         */

        Canvas canvas = new Canvas();
        canvas.setBackground(Color.white);
        canvas.setSize(i*cellSize, j*cellSize);
        frame.add(canvas);
        Graphics graphics = canvas.getGraphics();

        /**
         * Create the initial ocean.
         */

        sea = new Ocean(i, j, starveTime);

        /**
         * Visit each cell (in a roundabout order); randomnly place a fish, shark,
         * or nothing in each.
         */

        Random random = new Random(0);
        int x = 0;
        int y = 0;
        for(int row = 0;row < j; row++){
            //This will visit every x-coordinate once.
            x = (x + 78887) %j; 

            if((x & 8) == 0){
                for(int col = 0; col < i; col++){
                    //This will visit every y coordinate once.
                    y = (y+78887)%i;

                    if((y & 8) == 0){
                        int r = random.nextInt();

                        if(r < 0){
                            //50% of cells start with fish
                            //x - width, y - height
                            sea.addFish(x, y);
                        }else if(r > 1500000000){
                            //~15% of cells start with sharks
                            sea.addShark(x, y);
                        }
                    }
                }
            }
        }



        /**
         * Perform timesteps forever
         */

        while (true) {                           
            // Wait one second (1000 milliseconds)
              Thread.sleep(1000);                
           // Draw the current ocean
              drawOcean(graphics, sea);                       
              //  For fun, you might wish to change the delay in the next line.
              //  If you make it too short, though, the graphics won't work properly.
              // Simulate a timestep
              sea = sea.timeStep(); 

            }


    }

}
share|improve this question
1  
This is the same simulation they used in AP Computer Science courses over eight years ago... –  user505255 Mar 5 at 7:45
    
What's the deal with bitwise complements: d + ~(~n % d)? –  abuzittin gillifirca Mar 5 at 8:27
    
I would like someone disentangle the conditional spaghetti in timeStep() method. I suspect it might be essential spaghetti. –  abuzittin gillifirca Mar 5 at 8:38
    
@abuzittingillifirca 'd + ~(~n % d) ' is used to perform modulo operation with euclidean divison principle when n<0 and java perform real division so it works for positive n only, am sorry what does it mean, when you say, 'what's the deal'? so going back to problem description mentioned in the link above, we are resolving this problem:"You can also refer to locations such as (4, 0) or (-4, 3.......wrapping around at the edges." –  overexchange Mar 5 at 9:14
1  
@user3317808 Spaghetti comment is addressed at reviewers and not you. I count 5 levels of nested scopes and 100+ lines. I can't count the cyclomatic complexity by hand. I want to see and learn how other people deal with too many conditionals (= spaghetti conditional). –  abuzittin gillifirca Mar 5 at 9:50

3 Answers 3

Just a quick comment: Your Ocean class knows/does too many things.

I'd be expecting a Fish class and a Shark class. Creating an ocean requires a starveTime constructor parameter? That's a sign you've broken the single responsibility principle (SRP).

Methods like void addFish(int x, int y) would be void addFish(Fish fish) - let the fish know where he is in the ocean, and let the ocean know it has fish.

share|improve this answer
    
starveTime constructor? I think i set this value in Ocean() constructor. Are you saying that it does not make sense? –  overexchange Mar 5 at 9:30
    
Do you mean, I can have an Ocean class and then interface that can subclass Shark Fish etc...? –  overexchange Mar 5 at 9:40
3  
@user3317808: The answer is suggesting that your Ocean class contain Shark and Fish objects. The sharks would know how hungry they are. The ocean shouldn't care; It's just the stage where your actors play their parts. –  cHao Mar 5 at 13:40
    
@cHao When you say "Ocean class contain Shark and Fish objects." Are you saying, Shark and Fish should be inner class of Ocean? i think they Shark and Fish cannot be subclass of Ocean. –  overexchange Mar 6 at 7:15
    
@user3317808: There's no real need for them to be inner classes. The point is that you'd have a Fish class and a Shark class somewhere, and an Ocean would contain a collection of Shark and Fish instances. (Maybe separate, maybe in the same collection.) –  cHao Mar 6 at 9:02

Minor notes

// need to think on optimization this is really not that important, but if you're using an IDE, you could probably use an //TODO. This can be shown in most IDE and is more easily visible IMHO.

Javadoc

 * Ocean() is a constructor that creates an empty ocean having width i and
 * height j, in which sharks starve after starveTime timesteps.

I don't like that you use Ocean(), since at first I was thinking that it was a documentation for an empty constructor, when in fact it is not. You could only say :

 * Constructor that creates an empty ocean having width i and
 * height j, in which sharks starve after starveTime timesteps.

For my personal taste, there is too much documentation. When you add a javadoc for width(), you've been a step too far. This is really my opinion and it all depends on standard that you must follow.

Constant

Good thing, you're using constant! This is a first step in extracting concept in general! But there is still magic numbers in the code.

y = (y + 78887)

Why 78887 ? You're re-using it more than one time, so if you ever need to change it, you need to remember every occurrence of it! This is not the only one so you should look for it. In many case, even if you're using the number only once, it's easier and more readable to name a variable and use it instead!

Naming

 * @param i
 *            is the width of the ocean.
 * @param j
 *            is the height of the ocean.
 * @param starveTime
 *            is the number of timeSteps sharks survive without food.
 */

public Ocean(int i, int j, int starveTime) {

You should not use i and j here. In your documentation you're giving a meaning to those variables, why not use it! Even if they will be use in a loop later, you can name it width and height. If you need to differentiate the argument one from the private one, simply use this.

share|improve this answer
1  
78887 magic number is used in simtext.java and simulation.java which are test programs or driving programs to test Ocean class. –  overexchange Mar 5 at 9:37
    
Well in that case, it could be a static final variable in the class it make more sense to have! –  Marc-Andre Mar 5 at 13:30

As I mentioned in this answer, your Ocean class is breaking SRP, which makes your code harder to maintain than it needs to be.

Memory Usage

Let's pretend the ocean is an array of X-Y coordinates with 3 possible values at each intersection: EMPTY, FISH or SHARK (what your code does). If the ocean is 1000x1000 you have 1 million of these values in memory, and unless there's a massive over-population of fish or sharks, the vast majority of these in-memory values are EMPTY and serve no purpose: you have essentially defined an ocean as a... bitmap.

Maintainability

Let's pretend you have a new requirement, that a fish will move in either direction (randomly) at every unit of time. With your current code, you have a major problem: the ocean needs to "scan" itself entirely, and whenever it finds a FISH, it must compute a new coordinate for that fish and assign a new location... but that's quite tricky to do in a bitmap that you're iterating.

If you had a base class for anything that can exist at a specific location in the ocean:

public class Critter
{
    public Point location; // define getter and setters

    public void update()
    {
        // no-op on base class
    }
}

And then you could have a Fish class:

public class Fish extends Critter
{
    @Override
    public void update()
    {
        // compute a new value for the Location property
    }
}

..And a Shark class:

public class Shark extends Fish
{
    public int hungerLevel; // define getter and setters

    public Shark()
    {
        hungerLevel = 0;
    }

    private void eat(Fish fish)
    {
        hungerLevel = 0;
        // let the ocean know the fish is gone
    }

    @Override
    public void update()
    {
        // compute a new location - find nearest fish?
        hungerLevel++;
    }
}

Now you can have hunting sharks! Better yet, the Ocean class can now focus on being an ocean:

public class Ocean
{
    private List<Critter> _critters;

    public void update()
    {
        for(Critter critter : _critters)
        {
            critter.update();
        }
    }
}

Now obviously an updating Shark will want to know if there are any other critters nearby, and an updating Fish will probably want to try to escape a Shark (but the Shark could be moving faster!), so perhaps the Critter.update() method should take an Iterable<Critter>:

public class Fish extends Critter
{
    public void update(Iterable<Critter> critters)
    {
        // compute a new value for the Location property
    }
}

So the Ocean could pass in its critters:

public class Ocean
{
    private List<Critter> _critters;

    public void update()
    {
        for(Critter critter : _critters)
        {
            critter.update(_critters);
        }
    }
}

And now you can implement the Shark's hunting behavior, and the Fish's swim-for-your-life behavior

public class Shark extends Fish
{
    public Shark()
    {
        hungerLevel = 0;
    }

    public int hungerLevel; // define getter setters

    private void eat(Fish fish)
    {
        hungerLevel = 0;
        // let the ocean know the fish is gone
    }

    private Fish _currentTarget;

    @Override
    public void update(Iterable<Critter> critters)
    {
        // are we hunting a target already?
        if (_currentTarget != null)
        {
            // try to get closer.. and try to eat!
            return;
        }
        else
        {
            // is there a fish nearby?
            for(Critter critter : critters)
            {
                // if there's a fish (viz not a shark) in sight, make it our target.
                if (!(critter instanceof Shark) && canCapture(critter))
                {
                     _currentTarget = critter;                         
                }
            }
        }

        // not eating; increment hunger level:
        hungerLevel++;
    }
}

Obviously this is just food for thought, I'm not going to implement the whole thing here, but you get the idea: now you can decide that a Fish that weighs 20 pounds will have a bigger impact on a Shark's appetite than a Fish that weighs 2 pounds, for example: the Ocean class couldn't care less about these details - and it's up to you to put that code where it belongs.

Yes, this might look like it's more code to write. It could very well be. But I've gone overboard here, just to illustrate a point. The bitmap is gone, the ocean doesn't need to scan itself anymore, and you're only storing the data that you need; if there's 1 fish in the entire ocean, finding it will be much, much, much faster that way - and with each class responsible for its own business, extending your simulation and adding more variables and possibly other critters in the food chain, wouldn't be very hard.

share|improve this answer
    
Can I edit your answer to make it correct in Java's term? –  tintinmj Mar 5 at 19:40
    
@tintinmj sure! –  Mat's Mug Mar 5 at 19:41
    
Changed IsInsight to canCapture. One thing also I didn't understand why make eat a private method and not include in the base class? –  tintinmj Mar 5 at 20:02
    
@tintinmj got caught by the assumption that only Sharks can eat - perhaps eat(Critter critter) would make it more extensible indeed :) –  Mat's Mug Mar 5 at 20:04
1  
Good job at implementing the problem btw - it's just that the exercise isn't object-oriented. –  Mat's Mug Mar 6 at 10:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.