4
\$\begingroup\$

I'm trying to clone a Dots game with some little differences (power ups and objectives) as a school project. I have almost every objective done so now, I'm not trying to get my work done for free. I am however asking for code structure advice and to learn if you find any horrible mistake. I'm sorry that the code isn't commented, maybe a bad habit but I prefer to make the code as readable as I can without them (at least I hope it is readable).

Also, pt.isel.a41530.lib.tile package is not my code and I'm not asking for a review of that code.

GitHub

My main concerns are:

  • Redundant code in the game logic, especially with the combo having objects with x,y when I have a bi-dimensional array.
  • Too much work to add new Pieces:
  • Creating a class for the object, creating a type (on the enum) but that static reference doesn't seem good to me, it still seems better than creating an object every time I want to check the object characteristics.
  • The class for the GameLogic seems to have to much work: The game logic is divided in 2 classes, one handles all the "base game mechanics" and the other handles the score/objectives/playsleft/save/load. Should I split them a bit more?
  • Visual characteristics of the pieces are on the "model" (that should only handle logic and be completely portable if I might want to make the game for desktop or something along those lines).

Game Base Logic: pt/isel/a41530/linkeddots/model/LinkedDots.java

package pt.isel.a41530.linkeddots.model;

import java.io.BufferedWriter;
import java.io.IOException;
import java.util.LinkedList;

import pt.isel.a41530.linkeddots.model.pieces.Piece;
import pt.isel.a41530.linkeddots.model.pieces.PieceType;

public class LinkedDots {

    private static final boolean SIMPLE = false, CONNECTED = true, FINISH = true;

    private final int COLS, LINES;
    private PieceType[][] grid;
    private LinkedList<Piece> connectedPieces = new LinkedList<>();

    private LevelManager manager;
    private OnActionListener listener;

    public LinkedDots(){
        this(new LevelManager());
    }

    public LinkedDots(LevelManager manager){
        this.manager = manager;
        COLS = manager.getCols();
        LINES = manager.getLines();
        grid = new PieceType[COLS][LINES];
        manager.newLevel(this);
    }

    public boolean newCombo(int c, int l){
        if(grid[c][l].sample().isBasic()){
            addPieceToCombo(c, l);
            return true;
        }
        return false;
    }

    public boolean addLink(int c, int l){
        if(validMovement(c, l)){
            if(checkUndo(c, l)){
                listener.undo(connectedPieces.getLast().getX(), connectedPieces.getLast().getY());
                connectedPieces.removeLast();
            }
            else if(checkComboCollision(c, l)){
                if(specialCombo(c, l))
                    finishCombo(CONNECTED);
            }
            else if (checkTypes(c, l)){
                addPieceToCombo(c, l);
                return true;
            }
        }
        return false;
    }

    private boolean checkTypes(int c, int l){
        return grid[c][l] == connectedPieces.getFirst().getType()
                || grid[c][l].sample().alwaysConnects();
    }

    private boolean validMovement(int c, int l){
        int x = connectedPieces.getLast().getX();
        int y = connectedPieces.getLast().getY();
        return (Math.abs(c - x) == 1 && l - y == 0) || (Math.abs(l - y) == 1 && c - x == 0);
    }

    private boolean checkUndo(int c, int l){
        return connectedPieces.size() > 1 &&
                c == connectedPieces.get(connectedPieces.size()-2).getX() &&
                l == connectedPieces.get(connectedPieces.size()-2).getY();
    }

    private boolean checkComboCollision(int c, int l) {
        for(Piece piece : connectedPieces){
            if(piece.getX() == c && piece.getY() == l)
                return true;
        }
        return false;
    }

    private boolean specialCombo(int c, int l){
        return c == connectedPieces.getFirst().getX()
                && l == connectedPieces.getFirst().getY();
    }

    public boolean finishCombo(){
        if(connectedPieces.size()>1) {
            finishCombo(SIMPLE);
            return true;
        }
        listener.undo(connectedPieces.getFirst().getX(), connectedPieces.getFirst().getY());
        resetCombo();
        return false;
    }

    private void finishCombo(boolean type){
        char pieceType = connectedPieces.getFirst().getType().getID();
        int counter = clearCombo();
        if(type)                                                    //type == CONNECTED
            counter += clearAllPieces(connectedPieces.getFirst().getType());
        newPieces();
        resetCombo();
        if(manager.play(counter, pieceType) != FINISH) {
            listener.play();
        }else
            listener.onGameFinish();
    }

    private int clearCombo(){
        for (Piece piece: connectedPieces){
            clearPiece(piece.getX(), piece.getY());
        }
        return connectedPieces.size();
    }

    private int clearAllPieces(PieceType type){
        int counter = 0;
        for (int c = 0; c < COLS; c++)
            for(int l = LINES - 1; l >= 0; l--)
                if(grid[c][l] != null && grid[c][l] == type) {
                    clearPiece(c, l);
                    counter++;
                }
        return counter;
    }

    private void newPieces(){
        boolean hasNext;
        for (int c = 0; c < COLS; c++) {                    //for each column
            hasNext = true;
            for (int l = LINES-1; l >= 0; l--) {            //for each line
                if(grid[c][l] == null && hasNext)           //check if is null(and still "has" next)
                    hasNext = checkCol(c, l);               //check if truly has next and shift
                if(!hasNext && grid[c][l] == null) {        //if there is no more pieces
                    grid[c][l] = manager.newRandomPiece();  //bring a new random piece
                    listener.newPiece(c, l);                //notify that there is a new piece
                }
            }
        }
    }

    private boolean checkCol(int column, int lineOffset){
        for(int line = lineOffset - 1; line >= 0; line--){  //for each line (starting in the offset)
            if(grid[column][line] != null){                 //if the line != null "hasNext"
                if(grid[column][line].sample().isMovable()) {
                    /* Shift Piece in model and view */
                    grid[column][lineOffset] = grid[column][line];
                    listener.shiftPiece(column, line, lineOffset);
                    grid[column][line] = null;
                    return true;
                }
            }
        }
        return false;
    }

    private void addPieceToCombo(int c, int l){
        connectedPieces.add(PieceType.newPiece(grid[c][l], c, l));
    }
    public void addPiece(char type, int c, int l){
        grid[c][l] = PieceType.valueOf(type);
    }
    public void addPiece(PieceType type, int c, int l){
        grid[c][l] = type;
    }

    private void resetCombo(){
        connectedPieces = new LinkedList<>();
    }
    private void clearPiece(int c, int l) {
        grid[c][l] = null;
        listener.clearPiece(c, l);
    }

    public PieceType getPiece(int c, int l) {
        return grid[c][l];
    }

    public int getCols(){
        return COLS;
    }
    public int getLines(){
        return LINES;
    }
    public int getScore(){
        return manager.getScore();
    }
    public int getPlays() {
        return manager.getPlaysRemaining()>0? manager.getPlaysRemaining() : 0;
    }

    public void saveLevel(BufferedWriter writer) throws IOException {
        manager.saveLevel(writer, this);
    }
    public void setOnPlayListener(OnActionListener listener){
        this.listener = listener;
    }

    @Override
    public String toString(){
        String model = "";
        for (int l = 0; l < LINES; l++) {
            for (int c = 0; c < COLS; c++) {
                model += grid[c][l].getID() + " ";
            }
            model += "\n";
        }
        return model;
    }
}

Game Score Logic (plus Save/Load) pt/isel/a41530/linkeddots/model/LevelManager.java

package pt.isel.a41530.linkeddots.model;

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.IOException;

import pt.isel.a41530.linkeddots.model.pieces.PieceType;

public class LevelManager {

    public static final int COLS = 6, LINES = 6, PLAYS_REMAINING = 15, SCORE = 0;
    private final int cols, lines;
    private int score;
    private int playsRemaining;
    private int totalPieceTypes;
    private Objective[] objectives;
    private String[] initialData;

    public LevelManager(){
        cols = COLS;
        lines = LINES;
        playsRemaining = PLAYS_REMAINING;
        score = SCORE;
        totalPieceTypes = 6;
    }

    public LevelManager(int cols, int lines, int playsRemaining, int score){
        this.cols = cols;
        this.lines = lines;
        this.playsRemaining = playsRemaining;
        this.score = score;
        totalPieceTypes = 3;
    }

    public LevelManager(int cols, int lines, int playsRemaining, int score, int totalPieceTypes){
        this(cols, lines, playsRemaining, score);
        this.totalPieceTypes = totalPieceTypes;
    }

    public boolean play(int score, char type){
        this.score += score;
        for (Objective objective : objectives)
            if(objective.getType() == type)
                objective.increase(score);
        return playsRemaining--<=0;
    }

    public static LevelManager loadLevel(BufferedReader reader) throws IOException{
        String[] line = reader.readLine().split("[ x]");
        int cols = Integer.parseInt(line[0]);
        int lines = Integer.parseInt(line[1]);
        int playsRemaining = Integer.parseInt(line[2]);
        int score = Integer.parseInt(line[3]);
        LevelManager manager = new LevelManager(cols, lines, playsRemaining, score);

        /* Seconds Line : Objectives */
        String s = reader.readLine();
        if(s.length()>1) {
            line = s.split(" ");
            Objective[] objectives = new Objective[line.length];
            for (int i = 0; i < line.length; i++) {
                char c = line[i].charAt(0);
                String objective = "";
                String progress = "";
                for (int l = 1; l < line[i].length(); l++) {
                    if (line[i].charAt(l) == '/') {
                        for (l++; l < line[i].length(); l++) {
                            progress += line[i].charAt(l);
                        }
                    } else objective += line[i].charAt(l);
                }
                objectives[i] = new Objective(c, Integer.valueOf(objective));
                if (progress.length() > 0) {
                    objectives[i].setProgress(Integer.valueOf(progress));
                }
            }
            manager.setObjective(objectives);
        }

        /* New Pieces */
        manager.initialData = new String[lines];
        for(int l = 0; l < lines && (s = reader.readLine())!=null; l++) {
            manager.initialData[l] = s.replace(" ", "");
        }

        return manager;
    }

    public void saveLevel(BufferedWriter writer, LinkedDots model) throws IOException{
        writer.write(cols + "x" + lines + " " + playsRemaining + " " + score);
        writer.newLine();
        if(objectives!=null) {
            for (Objective obj : objectives) {
                writer.write(obj.toString() + " ");
            }
        }
        writer.newLine();
        writer.write(model.toString());
    }

    public void newLevel(LinkedDots model) {
        if(initialData != null){
            dataLevel(model);
        }else{
            randomLevel(model);
        }
    }

    private void dataLevel(LinkedDots model) {
        for(int l = 0; l < lines; l++)
            for (int c = 0; c < cols; c++)
                model.addPiece(initialData[l].charAt(c), c, l);
    }

    private void randomLevel(LinkedDots model){
        for (int c = 0; c < cols; c++)
            for(int l = 0; l < lines; l++)
                model.addPiece(newRandomPiece(), c, l);
    }

    public PieceType newRandomPiece(){
        return PieceType.createPiece((int)(Math.random() * totalPieceTypes));
    }

    private void setObjective(Objective[] obj){
        this.objectives = new Objective[obj.length];
        System.arraycopy(obj, 0, this.objectives, 0, obj.length);
    }

    public int getCols(){
        return cols;
    }
    public int getLines(){
        return lines;
    }
    public int getScore(){
        return score;
    }
    public int getPlaysRemaining(){
        return playsRemaining;
    }
    public int getPieceTypes() {
        return totalPieceTypes;
    }
    public Objective[] getObjectives(){
        return objectives;
    }

    public static class Objective{

        private final char type;
        private final int objective;
        private int progress = 0;

        private Objective(char type, int amount) {
            this.type = type;
            this.objective = amount;
        }

        public boolean increase(int progress){
            return (this.progress += progress) >= objective;
        }

        public char getType() {
            return type;
        }

        public int getObjective() {
            return objective;
        }

        public int getProgress(){
            return progress;
        }

        public void setProgress(int progress) {
            this.progress = progress;
        }

        @Override
        public String toString(){
            String obj = String.valueOf((progress == 0)? objective : (objective + "/" + progress));
            return type + obj;
        }
    }
}

PieceTypes Enum: pt/isel/a41530/linkeddots/model/pieces/PieceType.java

package pt.isel.a41530.linkeddots.model.pieces;

public enum PieceType {

    RED('R'){

        protected Piece newPiece(int c, int l){
            return new BasicPiece(c, l, getID());
        }

        public Piece sample(){
            return redPieceSample;
        }

    }, GREEN('G'){

        protected Piece newPiece(int c, int l){
            return new BasicPiece(c, l, getID());
        }

        public Piece sample(){
            return greenPieceSample;
        }

    }, BLUE('B'){

        protected Piece newPiece(int c, int l){
            return new BasicPiece(c, l, getID());
        }

        public Piece sample(){
            return bluePieceSample;
        }

    }, AllType('?'){

        protected Piece newPiece(int c, int l) {
            return new AllTypePiece(c, l);
        }

        public Piece sample() {
            return allTypePieceSample;
        }

    }, BLOCK('#'){

        protected Piece newPiece(int c, int l) {
            return null;
        }

        public Piece sample() {
            return blockPieceSample;
        }

    }, N_BLOCK('X'){

        protected Piece newPiece(int c, int l) {
            return null;
        }

        public Piece sample() {
            return noGravityBlockSample;
        }

    };


    private final char NAME_ID;

    private PieceType(char nameID){
        this.NAME_ID = nameID;
    }

    protected abstract Piece newPiece(int c, int l);
    public abstract Piece sample();

    public static PieceType valueOf(char c){
        for(PieceType dot: values()){
            if(dot.NAME_ID == c) {
                return dot;
            }
        }
        return null;
    }

    public char getID(){
        return NAME_ID;
    }

    public static PieceType createPiece(int rng){
        for(PieceType type: values()){
            if(type.ordinal() == rng) {
                return type;
            }
        }
        return null;
    }

    public static Piece newPiece(PieceType type, int c, int l) {
        return type.newPiece(c, l);
    }

    private static Piece redPieceSample = new BasicPiece('R');
    private static Piece greenPieceSample = new BasicPiece('G');
    private static Piece bluePieceSample = new BasicPiece('B');
    private static Piece allTypePieceSample = new AllTypePiece();
    private static Piece blockPieceSample = new BlockPiece();
    private static Piece noGravityBlockSample = new NoGravityBlockPiece();
}
\$\endgroup\$
  • \$\begingroup\$ Adding the relevant parts of the code to be reviewed is mandatory. Links to other only hosting sites are ok, but the main parts you would like reviewed must be embedded here in the question body. \$\endgroup\$ – glampert Jun 17 '15 at 18:03
  • 1
    \$\begingroup\$ Your indentation has issues. I find it hard to review this code. \$\endgroup\$ – Pimgd Oct 28 '15 at 14:34
  • \$\begingroup\$ @Pimgd Your comment has caused an edit suggestion of a code edit. Please also add the fact that we cannot edit code (it is OP's job), so that this does not happen again. \$\endgroup\$ – TheCoffeeCup Nov 1 '15 at 18:55
  • \$\begingroup\$ @MannyMeng are you suggesting I edit an old comment of mine (which you can't) to include a message which you yourself could post, since you're writing a comment anyway? Anyway, I don't think the code with bad indentation was reviewable (too much effort). \$\endgroup\$ – Pimgd Nov 2 '15 at 8:53
  • \$\begingroup\$ I'm sorry but where do you see the bad code indentation? Also, thanks anyway but my implementation changed a lot since I did this so this isn't really relevant anymore. \$\endgroup\$ – Pedro David Nov 2 '15 at 11:09
1
\$\begingroup\$

I saw your comment about the question wasn't relevant anymore, but still wanted to answer this one.
I have a deep love for Java enums and this I couldn't resist to answer.

Your trying to create a factory pattern inside an enum.
I do think this isn't the way to go, but I want to point out some issues so leaving this construction working as it is.

You work with the static variable to return them in the implemented method sample(). You could refactor it to :

RED('R',new BasicPiece('R')){

    @Override
    protected Piece newPiece(int c, int l){
        return new BasicPiece(c, l, getID());
    }
}

Where your contructor also change to :

private PieceType(char nameID, Piece piece){
    this.NAME_ID = nameID;
    this.piece = piece;
}

and the sample method just return piece.
This saves already a lot of lines code and does the same thing as your code.

Now there is 1 big issue with your and mine refactored code.
An Enum should be a constant, but the sample can change.

For example take this code :

((BasicPiece)PieceType.RED.sample()).setChar('G');

Of course I don't know if there is a setter provided for the char, but it works with every setter.
At this moment you change the object in the enum and next time you ask a sample you get the altered version.

If you want to proceed this way I suggest returning a new Piece with sample could be the best way.
The cleanest way is to change the new BasicPiece() to a more factory based system like new BasicPieceFactory() who extends a factory that implements createSample method.

Like this you don't need to override the sample method in the enum but it can change to this :

public Piece sample() {
    return factory.createSample();
}

Also with this method you could move the method newPiece to the factory.

Footnote :

I did check your github and was happy to see the refactored enum. It's more logic in that way with separating the concerns.

\$\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.