10
\$\begingroup\$

I just coded a version of WaTor, a population simulation game in which fish and sharks move around, breed, and eat each other. I am wondering how I did and if there are better ways to do some of the things I am doing, specifically on the move methods for the sharks and the fish.

Animal

package WaTor;

abstract class Animal {

private int x;
private int y;
private int type;
private int SIZE;

Animal(int xcoord, int ycoord, int t, int size) {
    x=xcoord;
    y=ycoord;
    type=t;
    SIZE = size;
}

public int getX() {
    return x;
}

public int getY() {
    return y;
}

public int getType() {
    return type;
}

public void moveX(int dx) {
    x = x + dx;
    if(x > SIZE - 1) {
        x = 0;
    }
    if(x < 0) {
        x = SIZE - 1;
    }
}

public void moveY(int dy) {
    y = y + dy;
    if(y > SIZE - 1) {
        y = 0;
    }
    if(y < 0) {
        y = SIZE - 1;
    }
}

public void moveUp() {
    moveX(-1);
}

public void moveDown() {
    moveX(1);
}

public void moveLeft() {
    moveY(-1);
}

public void moveRight() {
    moveY(1);
}

abstract boolean canBreed();
abstract boolean hasStarved();
}

Fish

package WaTor;

import java.util.Random;

class Fish extends Animal {

private static final int TYPE = 1; 
private static final int canBreed = 4;
private int breedAge;


Fish(int xcoord, int ycoord, int size) {
    super(xcoord,ycoord,TYPE,size);
    breedAge = 0;
}

public void moveUp() {
    moveX(-1);
    breedAge ++;
}

public void moveDown() {
    moveX(1);
    breedAge ++;
}

public void moveLeft() {
    moveY(-1);
    breedAge ++;
}

public void moveRight() {
    moveY(1);
    breedAge ++;
}

public boolean canBreed() {
    return breedAge == canBreed;
}

public void incBreedAge() {
    breedAge++;
}

public boolean hasStarved() {
    return false;
}

}

Shark

package WaTor;

import java.util.Random;

class Shark extends Animal {

private static final int TYPE = 2;
private static final int hasStarved = 4;
private static final int canBreed = 2;

private int starveTime;
private int energy;

Shark(int xcoord, int ycoord, int size) {
    super(xcoord,ycoord,TYPE,size);
    starveTime = 0;
    energy = 0;
}

public void moveUp() {
    moveX(-1);
    starveTime ++;
}

public void moveDown() {
    moveX(1);
    starveTime ++;
}

public void moveLeft() {
    moveY(-1);
    starveTime ++;
}

public void moveRight() {
    moveY(1);
    starveTime ++;
}

public boolean hasStarved() {
    return starveTime == hasStarved;
}

public boolean canBreed() {
    return energy == canBreed;
}

public void resetEnergy() {
    energy = 0;
}

public void resetStarveTime() {
    starveTime = 0;
}

public void incEnergy() {
    energy++;
}

public void incStarveTime() {
    starveTime++;
}
}

Ocean

package WaTor;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.Random;

class Ocean {

private Animal[][] ocean;

private int numFish;
private int numSharks;
private int SIZE;

private ArrayList<Fish> fish;
private ArrayList<Shark> sharks;
private ArrayList<Shark> sharksToRemove;
private ArrayList<Shark> sharksToAdd;
private ArrayList<Fish> fishToAdd;

private Random random;

Ocean(int size, int f, int s) {
    SIZE = size;
    numFish = f;
    numSharks = s;
    random = new Random();
    ocean = new Animal[SIZE][SIZE];
    fish = new ArrayList<Fish>();
    sharks = new ArrayList<Shark>();
    sharksToRemove = new ArrayList<Shark>();
    sharksToAdd = new ArrayList<Shark>();
    fishToAdd = new ArrayList<Fish>();
    fillOcean();
    for(Fish fishy : fish) {
        addFish(fishy);
    }
    for(Shark sharky : sharks) {
        addShark(sharky);
    }
}

private void fillOcean() {
    for (int i = 0; i < numFish; i++) {
        int x = random.nextInt(SIZE);
        int y = random.nextInt(SIZE);
        addNewFish(x, y);
    }
    for (int j = 0; j < numSharks; j++) {
        int x = random.nextInt(SIZE);
        int y = random.nextInt(SIZE);
        addNewShark(x, y);
    }
}

private void addFish(Fish fish) {
    ocean[fish.getX()][fish.getY()] = fish;
}

private void addShark(Shark shark) {
    ocean[shark.getX()][shark.getY()] = shark;
}

private void addNewFish(int x, int y) {
    Fish aFish = new Fish(x, y, SIZE);
    if (!cantAdd(aFish)) {
        fish.add(aFish);
    } else {
        x = random.nextInt(SIZE);
        y = random.nextInt(SIZE);
        addNewFish(x, y);
    }
}

private void addNewShark(int x, int y) {
    Shark bruce = new Shark(x, y, SIZE);
    if (!cantAdd(bruce)) {
        sharks.add(bruce);
    } else {
        x = random.nextInt(SIZE);
        y = random.nextInt(SIZE);
        addNewShark(x, y);
    }
}

private boolean hasFishNeighborUp(Shark animal) {
    for (Fish fishy : fish) {
        if (((animal.getX() - 1) == fishy.getX())
                && (animal.getY() == fishy.getY())) {
            fish.remove(fishy);
            return true;
        }
    }
    return false;
}

private boolean hasFishNeighborDown(Shark animal) {
    for (Fish fishy : fish) {
        if (((animal.getX() + 1) == fishy.getX())
                && (animal.getY() == fishy.getY())) {
            fish.remove(fishy);
            return true;
        }
    }
    return false;
}

private boolean hasFishNeighborLeft(Shark animal) {
    for (Fish fishy : fish) {
        if ((animal.getX() == fishy.getX())
                && ((animal.getY() - 1) == fishy.getY())) {
            fish.remove(fishy);
            return true;
        }
    }
    return false;
}

private boolean hasFishNeighborRight(Shark animal) {
    for (Fish fishy : fish) {
        if ((animal.getX() == fishy.getX())
                && ((animal.getY() + 1) == fishy.getY())) {
            fish.remove(fishy);
            return true;
        }
    }
    return false;
}

private boolean hasNeighborUp(Animal animal) {
    for (Fish fishy : fish) {
        return (((animal.getX() - 1) == fishy.getX()) && (animal.getY() == fishy
                .getY()));
    }
    for (Shark sharky : sharks) {
        return (((animal.getX() - 1) == sharky.getX()) && (animal.getY() == sharky
                .getY()));
    }
    return false;
}

private boolean hasNeighborDown(Animal animal) {
    for (Fish fishy : fish) {
        return (((animal.getX() + 1) == fishy.getX()) && (animal.getY() == fishy
                .getY()));
    }
    for (Shark sharky : sharks) {
        return (((animal.getX() + 1) == sharky.getX()) && (animal.getY() == sharky
                .getY()));
    }
    return false;
}

private boolean hasNeighborLeft(Animal animal) {
    for (Fish fishy : fish) {
        return ((animal.getX() == fishy.getX()) && ((animal.getY() - 1) == fishy
                .getY()));
    }
    for (Shark sharky : sharks) {
        return ((animal.getX() == sharky.getX()) && ((animal.getY() - 1) == sharky
                .getY()));
    }
    return false;
}

private boolean hasNeighborRight(Animal animal) {
    for (Fish fishy : fish) {
        return ((animal.getX() == fishy.getX()) && ((animal.getY() + 1) == fishy
                .getY()));
    }
    for (Shark sharky : sharks) {
        return ((animal.getX() == sharky.getX()) && ((animal.getY() + 1) == sharky
                .getY()));
    }
    return false;
}

private boolean cantAdd(Animal animal) {
    if(fish.size()==0 && sharks.size()==0) {
        return false;
    }
    for (Fish fishy : fish) {
        return (animal.getX() == fishy.getX())
                && (animal.getY() == fishy.getY());
    }
    if(sharks.size()==0) {
        return false;
    }
    for (Shark sharky : sharks) {
        return (animal.getX() == sharky.getX())
                && (animal.getY() == sharky.getY());
    }
    return false;
}

private void moveShark(Shark shark) {
    int rand = random.nextInt(4);
    if (hasFishNeighborUp(shark)) {
        shark.moveUp();
        shark.incEnergy();
        shark.resetStarveTime();
    } else if (hasFishNeighborDown(shark)) {
        shark.moveDown();
        shark.incEnergy();
        shark.resetStarveTime();
    } else if (hasFishNeighborLeft(shark)) {
        shark.moveLeft();
        shark.incEnergy();
        shark.resetStarveTime();
    } else if (hasFishNeighborRight(shark)) {
        shark.moveRight();
        shark.incEnergy();
        shark.resetStarveTime();
    } else if (!hasFishNeighborUp(shark) && !hasFishNeighborDown(shark)
            && !hasFishNeighborLeft(shark) && !hasFishNeighborRight(shark)) {
        if(hasNeighborUp(shark) && hasNeighborDown(shark) && hasNeighborLeft(shark) && hasNeighborRight(shark)) {
            shark.incStarveTime();
        }
        else if(rand==0 && !hasNeighborUp(shark)) {
            shark.moveUp();
            shark.incStarveTime();
        }
        else if(rand==0 && hasNeighborUp(shark)) {
            rand = 1;
        }
        else if(rand==1 && !hasNeighborDown(shark)) {
            shark.moveDown();
            shark.incStarveTime();
        }
        else if(rand==1 && hasNeighborDown(shark)) {
            rand = 2;
        }
        else if(rand==2 && !hasNeighborLeft(shark)) {
            shark.moveLeft();
            shark.incStarveTime();
        }
        else if(rand==2 && hasNeighborLeft(shark)) {
            rand = 3;
        }
        else if(rand==3 && !hasNeighborRight(shark)) {
            shark.moveRight();
            shark.incStarveTime();
        }
        else if(rand==3 && hasNeighborRight(shark)) {
            moveShark(shark);
        }
    }
}

public void moveFish(Fish fish) {
    int rand = random.nextInt(4);
    if(hasNeighborUp(fish) && hasNeighborDown(fish) && hasNeighborLeft(fish) && hasNeighborRight(fish)) {
        fish.incBreedAge();
    }
    if(rand==0 && !hasNeighborUp(fish)) {
        fish.moveUp();
        fish.incBreedAge();
    }
    else if(rand==0 && hasNeighborUp(fish)) {
        moveFish(fish);
    }
    else if(rand==1 && !hasNeighborDown(fish)) {
        fish.moveDown();
        fish.incBreedAge();
    }
    else if(rand==1 && hasNeighborDown(fish)) {
        moveFish(fish);
    }
    else if (rand == 2 && !hasNeighborLeft(fish)) {
        fish.moveLeft();
        fish.incBreedAge();
    }
    else if(rand==2 && hasNeighborLeft(fish)) {
        moveFish(fish);
    }
    else if(rand == 3 && !hasNeighborRight(fish)) {
        fish.moveRight();
        fish.incBreedAge();
    }
    else if(rand==3 && hasNeighborRight(fish)) {
        moveFish(fish);
    }
}

private void updateSharks() {   
    for(Shark sharky : sharks) {
        if(sharky.hasStarved()) {
            sharksToRemove.add(sharky);
        }
        else if(sharky.canBreed()) {
            Shark newShark = new Shark(sharky.getX(),sharky.getY(), SIZE);
            sharksToAdd.add(newShark);
        }
        moveShark(sharky);
    }
    for(Shark aShark : sharksToRemove) {
        sharks.remove(aShark);
    }
    for(Shark theShark : sharksToAdd) {
        sharks.add(theShark);
    }
}

private void updateFish() {
    for(Fish fishy : fish) {
        if(fishy.canBreed()) {
            Fish newFish = new Fish(fishy.getX(),fishy.getY(),SIZE);
            fishToAdd.add(newFish);
        }
        moveFish(fishy);
    }
    for(Fish theFish : fishToAdd) {
        fish.add(theFish);
    }
}

public void update() {
    for(int i=0; i<SIZE; i++) {
        for(int j=0; j<SIZE; j++) {
            ocean[i][j]=null;
        }
    }
    updateSharks();
    updateFish();
    for(Fish fishy : fish) {
        addFish(fishy);
    }
    for(Shark sharky : sharks) {
        addShark(sharky);
    }
}

public void print() {
    for (int x = 0; x < ocean.length; x++) {
        for (int y = 0; y < ocean[0].length; y++) {
            if (ocean[x][y] == null) {
                System.out.print(" 0 ");
            } else {
                System.out.print(" "+ocean[x][y].getType()+" ");
            }
        }
        System.out.println("");
    }
}
}
\$\endgroup\$
  • \$\begingroup\$ private static final int canBreed = 4 is a little confusing. The name suggests that it should be a Boolean value, but it appears to be using a magic code to represent an ability? If 4 is a code, I'd use an enum to make its purpose clearer. \$\endgroup\$ – Carcigenicate Jun 18 '15 at 0:58
  • \$\begingroup\$ canBreed seems to be the age at which the animal has matured. A name like matureAge might be better. \$\endgroup\$ – Carcigenicate Jun 18 '15 at 1:00
13
\$\begingroup\$

Animal

private int SIZE;

Why do you start using uppercase here? It's no constant.

Animal(int xcoord, int ycoord, int t, int size) {
    x=xcoord;

Alternatively you can name our field xcoord and the constructor's argument x. It's equally wrong. Use just x for both and

   this.x = x;

This is the normal style and it's absolutely clear what argument corresponds with what field.

public void moveX(int dx) {
    x = x + dx;
    if(x > SIZE - 1) {
        x = 0;
    }
    if(x < 0) {
        x = SIZE - 1;
    }
}

This sounds wrong. It should be some kind of wrap-around, but with dx>1 or dx<-1, it can lead to surprising results. What you want is probably

public void moveX(int dx) {
    x = sanitize(x + dx);
}

public int sanitize(int z) {
    z %= SIZE;
    if (z<0) {
        z += SIZE;
    }
    return z;
}

I'm using here z as it works for both x and y. Note the spacing and the use of += instead of the more verbose = ... +.

public void moveUp() {
    moveX(-1);
}

Such methods are more than useless, unless you need to satisfy some external requirements. Why? They're pretty repetitive and every use of them makes your code even more repetitive.

Fish

public void moveUp() {
    moveX(-1);
    breedAge ++;
}

I've just said it. Even more repetitive.

If there really was a need for such a method, then it should look like

public void moveUp() {
    moveX(-1);
    afterMove();
}

and be defined in Animal with afterMove being implemented differently in Fish and Shark.

Shark

private ArrayList<Fish> fish;

    ...
    fish = new ArrayList<Fish>();

This should be

private final List<Fish> fish = new ArrayList<Fish>();

Note the use of final and List (rather than ArrayList).

Ocean(int size, int f, int s) {

Use proper variable names, especially in such important places as method declaration. Single letters (apart from things like x and y for coordinates) are generally frown upon. I consider them fine in something like

for(Fish f : fish) {
    addFish(f);
}

where they have no important and a very limited scope, but a method signature deserves something better. By using the convention

Ocean(int size, int numFish, int numShark) {
    this.size = size;
    ...

you save yourself the need to invent new names.

private void fillOcean() {
    for (int i = 0; i < numFish; i++) {
        int x = random.nextInt(SIZE);
        int y = random.nextInt(SIZE);
        addNewFish(x, y);
    }
    for (int j = 0; j < numSharks; j++) {
        ...
    }
}

I guess that recycling i would make it clearer than using a new variable name.

private void addNewFish(int x, int y) {
    Fish aFish = new Fish(x, y, SIZE);
    if (!cantAdd(aFish)) {
        fish.add(aFish);
    } else {
        x = random.nextInt(SIZE);
        y = random.nextInt(SIZE);
        addNewFish(x, y);
    }
}

I don't like this kind of recursion. Nobody knows how deep it gets and it's rather uncommon. Moreover, you repeat here what you did in fillOcean above. There's a simple solution

while (fish.size() < numFish) {
    int x = random.nextInt(size);
    int y = random.nextInt(size);
    Fish f = new Fish(x, y, size);
    if (canAdd(f)) {
        fish.add(f);
    }
}

I removed the double negation !cantAdd and renamed aFish to f. Names like aFish are rather uncommon, and I find something like newFish needless verbose (for a variable spanning just 3 lines).

private boolean hasFishNeighborUp(Shark animal) {

Again repetitive.... and why do you call the Shark just animal?

The name has... sound like a getter checking some condition.... but in this process the poor fish dies. So I'd rename it.

private boolean hasNeighborUp(Animal animal) ...

Another set of four. This all needs something like an

@RequiredArgsConstructor enum Direction {
    UP(+1, 0),
    ...;

    private final int dx;
    private final int dy;
}

and a method like

private boolean hasNeighbor(Animal animal, Direction direction) ...

and some loops

for (Direction d : Direction.values()) ...

private void moveShark(Shark shark) {

This is very very long. And a bit wrong:

} else if (!hasFishNeighborUp(shark) && !hasFishNeighborDown(shark)
        && !hasFishNeighborLeft(shark) && !hasFishNeighborRight(shark)) {

This condition seems to be trivially true as it's in an "else-if" and all the condition has been tested already.

    else if(rand==0 && hasNeighborUp(shark)) {
        rand = 1;
    }
    else if(rand==1 && !hasNeighborDown(shark)) {

Setting rand = 1 is a "dead store" as the condition below it won't get tested after it (simply "else-if").

Too long... skipping a bit.

for(Shark aShark : sharksToRemove) {
    sharks.remove(aShark);
}

You need no field sharksToRemove, it should be a local variable. This field grows and grows as sharks (to be removed) get added to it and never removed.

Use sharks.removeAll(sharksToRemove).

public void print()

Implement toString() instead, it's just a little bit longer and can be used in debugger and elsewhere.


private void addFish(Fish fish) {
    ocean[fish.getX()][fish.getY()] = fish;
}

This adds no fish as the List<Fish> fish does not change. It only updates ocean to reflect the fish's position.

public void update() ...

You first clean the ocean, then you do something else, and finally you repopulate the ocean. Can't you bring the two ocean operations together? Can't you reuse update in the constructor?

\$\endgroup\$
10
\$\begingroup\$

I'm not a Java expert, and I've only looked over this code a little bit, so I only have a few tips.

  • Why are your moveDirection methods in the specific animal classes inherited from Animal? I'd recommend implementing a simple move method in the animal class that allows the animal to move in a specific direction. Here's an example.

public void move(xAmount, yAmount) {
    this.xcoord += xAmount;
    this.ycoord += yAmount;
}

  • Some of your indentation is odd. You should have any code inside you classes indented. It looks much clearer than the current format.
  • Why have the coordinate variables named xcoord and ycoord? Simply naming them x and y would suffice.
  • Finally, add some comments describing what certain methods/classes do. It'll make reading your code much, much easier.

That's about all I can come up with. If there's anything else that you want me to cover, just mention it in the comments, and I'll see what I can do.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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