My getters and setters aren't wokring correctly

Question

As I'm going though the debugger it looks like it is working at first, however when I try calling getCoordinates() in my shipSunk() method, it returns a null value almost every time. What is wrong here?

public boolean shipSunk(ShipDerived[] s){
    ArrayListMultimap<Integer, Integer> temp;
    ArrayList<Integer> temp2 = new ArrayList<>();
    boolean sunk = false;
    for(int i=s.length-1; i>=0;i--){
        Ship st = s[i];
        temp = s[i].getCoordinates();  //returns null almost every time???
        temp2 = s[i].getCoordinatesList();


        for (Map.Entry<Integer, Integer> entry : temp.entries()){
            //System.out.println(entry.getKey() + ", " + entry.getValue());
            if(grid[entry.getKey()][entry.getValue()]=='x'){
                sunk = true;
            }
            else{
                sunk = false;
            }
        }

        if(sunk==true){
            System.out.println("Ship has been sunk!");
        }
    temp.clear();
    }
    return sunk;
}

And here is my Ship class (extended from abstract class) methods:

import java.util.ArrayList;

public class ShipDerived extends Ship{

    private String type;
    private int length;
    private ArrayListMultimap<Integer, Integer> coordinates = ArrayListMultimap.create();
    private ArrayList<Integer> c2 = new ArrayList<>();

    public ShipDerived(){
        this.type = type;
        this.length = length;
        this.coordinates = getCoordinates();
        this.c2 = getCoordinatesList();
    }

    public ShipDerived(String t, int l){
        this.type = t;
        this.length = l;
        this.coordinates = getCoordinates();
        this.c2 = getCoordinatesList();
    }

    @Override
    void setType(String t) {
        type = t;
    }

    @Override
    void setLength(int l) {
        length = l;
    }

    @Override
    String getType() {
        return type;
    }

    @Override
    int getLength() {
        return length;
    }

    @Override
    ArrayListMultimap<Integer, Integer> getCoordinates() {
        return this.coordinates;
    }

    ArrayList<Integer> getCoordinatesList() {
        return this.c2;
    }

    @Override
    void setCoordinates(int i, int j) {
        //coordinates.putAll(i, Collections.singleton(j));
        this.coordinates.put(i,j);
        this.c2.add(i);
        this.c2.add(j);
    }
}

Here is my Ship (abstract) class:

public abstract class Ship {
private int length;
private String type;
private ArrayListMultimap<Integer, Integer> coordinates;

Ship(){
    this.length = length;
    this.type = type;
}

abstract void setType(String t);

abstract void setLength(int l);

abstract String getType();

abstract int getLength();

abstract ArrayListMultimap<Integer, Integer> getCoordinates();

abstract void setCoordinates(int i, int j);

}

And this is what I am passing into my shipSunk() method. I an using getters/setters to create my ships:

p1board.shipSunk(p1.getShips()); //player 1
p2board.shipSunk(p2.getShips()); //player 2

These seem to work but here's some code into these as well:

public class Player {
private String name;
private ShipDerived[] ships = new ShipDerived[5];

public Player(){
    this.name = name;
    this.ships = ships;
}

public String getName(){
    return name;
}

public void setName(String x){
    name = x;
}

public void setShips(){
    int a = 0;
    for(int i=5; i>=1;i--){
        ShipDerived s = new ShipDerived("null", 0);
        Array.set(ships, a, s);
        a++;
    }
    ships[0].setType("carrier");
    ships[0].setLength(5);
    ships[1].setType("battleship");
    ships[1].setLength(4);
    ships[2].setType("destroyer");
    ships[2].setLength(3);
    ships[3].setType("submarine");
    ships[3].setLength(3);
    ships[4].setType("patrol boat");
    ships[4].setLength(2);
}

public ShipDerived[] getShips(){
    return ships;
}

Answer 1

You initialize this.coordinates twice. First as an inline field initializer and second in the constructor. The statement this.coordinates = getCoordinates(); does not make sense because the getter returns this.coordinates, so you effectively get this.coordinates = this.coordinates;

This shouldn't effect your issue, but my recommendation for easier debugging is to just set all fields that you can to final to avoid issues such as this and remove the superfluous call from the constructor.

Then I would investigate the ArrayListMultimap.create(); method, if there is any way that one returns null.

Answer 2

You have @Override annotation on getCoordinates() so I'm guessing your Ship class also have coordinates field. In shipSunk method you cast your object to Ship class so most likely it operate on field Ship.coordintes whis is null because you are initializing only DerivedShip.coordinates.

Check this for simple example of hiding fields: If you overwrite a field in a subclass of a class, the subclass has two fields with the same name(and different type)?

Answer 3

There are so many strange things in your code:

• Why has the Ship class private fields if you don't provide access methods for those fields? Without accessor methods nobody can use those fields, so just delete them!
• Why do the ShipDerived constructors initialize the coordinates and c2 fields if you already initialize them at the declaration?
• Why does Player.setShips() use some obscure Array.set(ships, a, s); when ships[a] = s; would be sufficient?
• Why do your constructors (for Ship, ShipDerived and Player) contain nonsense-code like this.type = type; (in ShipDeriveds default constructor) that seems important but does nothing?
• Why does the Player.setShips() method create empty ShipDerived instances and only afterwards sets their values?

After some cleanup your classes could look like:

Ship.java:

import com.google.common.collect.ArrayListMultimap;

public abstract class Ship {

    Ship(){
    }

    abstract void setType(String t);

    abstract void setLength(int l);

    abstract String getType();

    abstract int getLength();

    abstract ArrayListMultimap<Integer, Integer> getCoordinates();

    abstract void setCoordinates(int i, int j);
}

ShipDerived.java:

import java.util.ArrayList;
import com.google.common.collect.ArrayListMultimap;

public class ShipDerived extends Ship{

    private String type;
    private int length;
    private ArrayListMultimap<Integer, Integer> coordinates = ArrayListMultimap.create();
    private ArrayList<Integer> c2 = new ArrayList<>();

    public ShipDerived(){
    }

    public ShipDerived(String t, int l){
        this.type = t;
        this.length = l;
    }

    @Override
    void setType(String t) {
        type = t;
    }

    @Override
    void setLength(int l) {
        length = l;
    }

    @Override
    String getType() {
        return type;
    }

    @Override
    int getLength() {
        return length;
    }

    @Override
    ArrayListMultimap<Integer, Integer> getCoordinates() {
        return this.coordinates;
    }

    ArrayList<Integer> getCoordinatesList() {
        return this.c2;
    }

    @Override
    void setCoordinates(int i, int j) {
        this.coordinates.put(i, j);
        this.c2.add(i);
        this.c2.add(j);
    }
}

Player.java:

public class Player {
    private String name;
    private ShipDerived[] ships = new ShipDerived[5];

    public Player(){
    }

    public String getName(){
        return name;
    }

    public void setName(String x){
        name = x;
    }

    public void setShips(){
        ships[0] = new ShipDerived("carrier", 5);
        ships[1] = new ShipDerived("battleship", 4);
        ships[2] = new ShipDerived("destroyer", 3);
        ships[3] = new ShipDerived("submarine", 3);
        ships[4] = new ShipDerived("patrol boat", 2);
    }

    public ShipDerived[] getShips(){
        return ships;
    }
}

These changes might not yet solve your issue, but at least make your code easier to read and understand.

---

Actually, since the Ship class no longer contains any fields and has only abstract methods you could even turn it into an interface (but that would mean that your methods are now public instead of package private, which would mean that you would also need to declare them as public in ShipDerived).

Ship.java, interface version:

import com.google.common.collect.ArrayListMultimap;

public interface Ship {

    void setType(String t);

    void setLength(int l);

    String getType();

    int getLength();

    ArrayListMultimap<Integer, Integer> getCoordinates();

    void setCoordinates(int i, int j);
}