6
\$\begingroup\$

I've developed easy Game of Life project. I'd highly appreciate any feedback and possibilities to improve my code, both in efficiency and style. It works on list and it's terminal program.

ICell.java

public interface ICell {
   public int getX();
   public int getY();
}

Cell.java

public class Cell implements ICell {

private int x, y;

public Cell(int x, int y) {
    this.x = x;
    this.y = y;
}

public int getX() {
    return x;
}

public int getY() {
    return y;
}

@Override
public boolean equals(Object obj) {
    if (obj == null) {
        return false;
    }
    if (!Cell.class.isAssignableFrom(obj.getClass())) {
        return false;
    }
    final Cell other = (Cell) obj;
    if(this.x != other.getX() || this.y != other.getY())
        return false;

    return true;
}

@Override
public String toString() {
    return new String("(x: " + this.x + ", y: " + y + ")");
}

}

IGameOfLife.java

public interface IGameOfLife {
   public void setNewList(List<ICell> cells);
   public List<ICell> getLivingCells();
   public List<ICell> live();
   public List<ICell> getPresetCells(IInputMethod<Integer> sourceDataReader);
}

GameOfLife.java

public class GameOfLife implements IGameOfLife{
private List<ICell> livingCells = new ArrayList<ICell>();

public void setNewList(List<ICell> cells) {
    this.livingCells = cells;
    if(livingCells.isEmpty())
        System.out.print("No more living cells.\n");
}

public List<ICell> getLivingCells() {
    return livingCells;
}

public List<ICell> live() {
    IEngine engine = new GameOfLifeEngine(livingCells);

    // Checks if living cells will continue to live
     List<ICell> newList = engine.checkLivingCells();
    // Finds new cells reproduced by 3 nearby cells
    newList.addAll(engine.reproduction());

    return newList;
}

public List<ICell> getPresetCells(IInputMethod<Integer> sourceDataReader) {
    int amountOfInputData = sourceDataReader.getInput();
    List<ICell> presetCells = new ArrayList<ICell>();

    for (int i = 0; i < amountOfInputData; i++) {
        int x = sourceDataReader.getInput();
        int y = sourceDataReader.getInput();
        presetCells.add(new Cell(x, y));
    }

    return presetCells;
}

public static void main(String[] args) throws FileNotFoundException {
    GameOfLife gol = new GameOfLife();
    IInputMethod<Integer> sourceReader = new SourceDataReader(args);
    IInputMethod<String> userInput = new UserInputReader();
    IDisplayer gameOfLifeDisplayer;
    gol.setNewList(gol.getPresetCells(sourceReader));
    gameOfLifeDisplayer = new Displayer(gol.getLivingCells());

    do {
        gameOfLifeDisplayer.display();
        gol.setNewList(gol.live());
        gameOfLifeDisplayer.setNewCellsList(gol.getLivingCells());
    } while (!(userInput.getInput().equals("q") || (gol.getLivingCells().isEmpty())));

    sourceReader.closeInput();
    userInput.closeInput();
}
}

IGameOfLife.java

public interface IEngine {
   public List<ICell> checkLivingCells();
   public List<ICell> reproduction();
}

GameOfLifeEngine.java

public class GameOfLifeEngine implements IEngine {

private List<ICell> livingCells;
private List<ICell> deadCellsAroundLivingOnes;

public GameOfLifeEngine(List<ICell> cells) {
    this.livingCells = cells;
    deadCellsAroundLivingOnes = new ArrayList<ICell>();
}

public List<ICell> checkLivingCells() {
    List<ICell> newList = new ArrayList<ICell>();

    for (ICell cell : livingCells) {
        int neighbours = findAllLivingCellsAroundCoordinates(cell.getX(), cell.getY());
        if ((neighbours == 2 || neighbours == 3))
            newList.add(new Cell(cell.getX(), cell.getY()));
    }
    return newList;
}

public List<ICell> reproduction() {
    List<ICell> reproducedCells = new ArrayList<ICell>();
    for (ICell cell : livingCells)
        findAllDeadCellsAroundCoordinates(cell.getX(), cell.getY());

    for (ICell cell : deadCellsAroundLivingOnes) {
        if (findAllLivingCellsAroundCoordinates(cell.getX(), cell.getY()) == 3)
            reproducedCells.add(cell);
    }

    deadCellsAroundLivingOnes.clear();
    return reproducedCells;
}

private void addToEmptySpaces(ICell cell) {
    if (!deadCellsAroundLivingOnes.contains(cell))
        deadCellsAroundLivingOnes.add(cell);
}

private int findAllLivingCellsAroundCoordinates(int i, int j) {
    int neighbours = 0;
    if (livingCells.contains(new Cell(i - 1, j - 1)))
        neighbours++;
    if (livingCells.contains(new Cell(i, j - 1)))
        neighbours++;
    if (livingCells.contains(new Cell(i + 1, j - 1)))
        neighbours++;
    if (livingCells.contains(new Cell(i - 1, j)))
        neighbours++;
    if (livingCells.contains(new Cell(i + 1, j)))
        neighbours++;
    if (livingCells.contains(new Cell(i - 1, j + 1)))
        neighbours++;
    if (livingCells.contains(new Cell(i, j + 1)))
        neighbours++;
    if (livingCells.contains(new Cell(i + 1, j + 1)))
        neighbours++;
    return neighbours;
}

private int findAllDeadCellsAroundCoordinates(int i, int j) {
    int neighbours = 0;
    if (!livingCells.contains(new Cell(i - 1, j - 1)))
        addToEmptySpaces(new Cell(i - 1, j - 1));
    if (!livingCells.contains(new Cell(i, j - 1)))
        addToEmptySpaces(new Cell(i, j - 1));
    if (!livingCells.contains(new Cell(i + 1, j - 1)))
        addToEmptySpaces(new Cell(i + 1, j - 1));
    if (!livingCells.contains(new Cell(i - 1, j)))
        addToEmptySpaces(new Cell(i - 1, j));
    if (!livingCells.contains(new Cell(i + 1, j)))
        addToEmptySpaces(new Cell(i + 1, j));
    if (!livingCells.contains(new Cell(i - 1, j + 1)))
        addToEmptySpaces(new Cell(i - 1, j + 1));
    if (!livingCells.contains(new Cell(i, j + 1)))
        addToEmptySpaces(new Cell(i, j + 1));
    if (!livingCells.contains(new Cell(i + 1, j + 1)))
        addToEmptySpaces(new Cell(i + 1, j + 1));
    return neighbours;
}

IDisplayer.java

public interface IDisplayer {
   public void display();
   public void setNewCellsList(List<ICell> newCells);
}

Displayer.java

public class Displayer implements IDisplayer {

private List<ICell> livingCells;

public Displayer(List<ICell> cellsToDisplay) {
    this.livingCells = cellsToDisplay;
}

public void display() {
    String result = "";
    if (!livingCells.isEmpty()) {
        int horizontalMin = getMinimalHorizonalLivingCellPosition();
        int horizontalMax = getMaximalHorizonalLivingCellPosition();
        int verticalMin = getMinimalVerticalLivingCellPosition();
        int verticalMax = getMaximalVerticalLivingCellPosition();

        if (Math.abs(horizontalMax - horizontalMin) > 15 || Math.abs(verticalMax - verticalMin) > 70)
            System.out.println("Cells to far away from each other. Impossible to display.\nLiving cells: " + livingCells.size());
        else {
            horizontalMin -= 3;
            horizontalMax += 3;
            verticalMin -= 3;
            verticalMax += 3;

            for (int i = horizontalMin; i < horizontalMax; i++) {
                for (int j = verticalMin; j < verticalMax; j++)
                    result += (livingCells.contains(new Cell(i, j))) ? "1 " : "0 ";
                result += "\n";
            }

            System.out.println("x:(" + horizontalMin + ", " + horizontalMax + ")\ny:(" + verticalMin + ", "
                    + verticalMax + ")\n" + result);
        }
    }
}

public void setNewCellsList(List<ICell> newCells) {
    this.livingCells = newCells;
}

private int getMinimalHorizonalLivingCellPosition() {
    int min = livingCells.get(0).getX();
    for (ICell cell : livingCells)
        if (cell.getX() < min)
            min = cell.getX();
    return min;
}

private int getMaximalHorizonalLivingCellPosition() {
    int max = livingCells.get(0).getX();
    for (ICell cell : livingCells)
        if (cell.getX() > max)
            max = cell.getX();
    return max;
}

private int getMinimalVerticalLivingCellPosition() {
    int min = livingCells.get(0).getY();
    for (ICell cell : livingCells)
        if (cell.getY() < min)
            min = cell.getY();
    return min;
}

private int getMaximalVerticalLivingCellPosition() {
    int max = livingCells.get(0).getY();
    for (ICell cell : livingCells)
        if (cell.getY() > max)
            max = cell.getY();
    return max;
}

IInputMethod.java

public interface IInputMethod<E> {
    public E getInput();
    public void closeInput();
}

SourceDataReader.java

public class SourceDataReader implements IInputMethod<Integer> {

private final Scanner sourceReader;

public SourceDataReader(String[] args) throws FileNotFoundException {
    if (args.length == 1)
        sourceReader = setFileAsSource(args[0]);
    else
        sourceReader = setStdIOAsSource();
}

private Scanner setFileAsSource(String filename) throws FileNotFoundException {
    return new Scanner(new File(filename));
}

private Scanner setStdIOAsSource() {
    return new Scanner(System.in);
}

public Integer getInput() {
    return sourceReader.nextInt();
}

public void closeInput() {
    sourceReader.close();
}

UserInputReader.java

public class UserInputReader implements IInputMethod<String>{
private final Scanner userInput = new Scanner(System.in);

public String getInput() {
    System.out.print("Press any key to progress one generation (q to exit)... ");
    return userInput.hasNext() ? new String(userInput.next()) : "q";
}

public void closeInput() {
    userInput.close();
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Is there any point in having an ICell interface and a Cell base class, both of which are the same? You don't use the ICell anywhere apart from in Cell. Why not just have the base class? \$\endgroup\$
    – Savlon
    Mar 16 '16 at 13:44
  • \$\begingroup\$ Ok, this interface is not useful. What else should I change? \$\endgroup\$
    – Treck
    Mar 16 '16 at 16:19
  • \$\begingroup\$ I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Vogel612
    Mar 16 '16 at 22:26
2
\$\begingroup\$

Code duplication

private int findAllLivingCellsAroundCoordinates(int i, int j) {
    int neighbours = 0;
    if (livingCells.contains(new Cell(i - 1, j - 1)))
        neighbours++;
    if (livingCells.contains(new Cell(i, j - 1)))
        neighbours++;
    if (livingCells.contains(new Cell(i + 1, j - 1)))
        neighbours++;
    if (livingCells.contains(new Cell(i - 1, j)))
        neighbours++;
    if (livingCells.contains(new Cell(i + 1, j)))
        neighbours++;
    if (livingCells.contains(new Cell(i - 1, j + 1)))
        neighbours++;
    if (livingCells.contains(new Cell(i, j + 1)))
        neighbours++;
    if (livingCells.contains(new Cell(i + 1, j + 1)))
        neighbours++;
    return neighbours;
}

vs

private int findAllDeadCellsAroundCoordinates(int i, int j) {
    int neighbours = 0;
    if (!livingCells.contains(new Cell(i - 1, j - 1)))
        addToEmptySpaces(new Cell(i - 1, j - 1));
    if (!livingCells.contains(new Cell(i, j - 1)))
        addToEmptySpaces(new Cell(i, j - 1));
    if (!livingCells.contains(new Cell(i + 1, j - 1)))
        addToEmptySpaces(new Cell(i + 1, j - 1));
    if (!livingCells.contains(new Cell(i - 1, j)))
        addToEmptySpaces(new Cell(i - 1, j));
    if (!livingCells.contains(new Cell(i + 1, j)))
        addToEmptySpaces(new Cell(i + 1, j));
    if (!livingCells.contains(new Cell(i - 1, j + 1)))
        addToEmptySpaces(new Cell(i - 1, j + 1));
    if (!livingCells.contains(new Cell(i, j + 1)))
        addToEmptySpaces(new Cell(i, j + 1));
    if (!livingCells.contains(new Cell(i + 1, j + 1)))
        addToEmptySpaces(new Cell(i + 1, j + 1));
    return neighbours;
}

How can we avoid such repetition? The total number of cells around any cell is 8 and a cell is either alive or dead, so:

private int findAllDeadCellsAroundCoordinates(int i, int j) {
    return 8 - findAllLivingCellsAroundCoordinates(i, j);
}

Yes, I noticed that findAllDeadCellsAroundCoordinates also performed another job, but this means that it was performing two jobs, while the single responsibility principle suggests only one job per function. So write another function to findDeadCellsNearLivingOnes. When you expect a function to only retrieve an information and it has un-expected side effects debugging becomes a nightmare.

\$\endgroup\$
3
  • \$\begingroup\$ Oh man, sorry, I totally forgot to remove this whole neighbours counting from findAllDeadCellsAroundCoordinates(). Of course it's only purpose is finding all dead cells around and keep them in list. No idea, how to change it to not be so redundant. \$\endgroup\$
    – Treck
    Mar 16 '16 at 22:22
  • \$\begingroup\$ @Treck Of course it's only purpose is finding all dead cells around and keep them in list <- it is not because it also returns a count. \$\endgroup\$
    – Caridorc
    Mar 16 '16 at 22:25
  • \$\begingroup\$ It did when I first wrote it, then I changed it. I tried to update it here, but it got rolled back. So this method is just keeping dead cells in one list. That's all. No neighbourhood counting, I don't need amount of dead cell, just their localization. \$\endgroup\$
    – Treck
    Mar 16 '16 at 22:26

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.