6
\$\begingroup\$

I'm pretty new to programming (haven't written much code myself, only read code for my computer science studies etc) and I keep getting confused by how to best structure my code.

Let me get a bit more specific by including the code of a Chess game I started to program today (want it to fully function text based, GUI maybe later):

/**    
 * Created by Max on 03/08/2016.
 */
    public class Figures {

    public String name;
    private Field fieldObject = new Field('0', 0);

    public Figures(String name) {                                
    this.name = name;
    };

    public void move(Field src, Field dst) {   // get name and the color of figures from src and dst + calls another
        String figurename = StringfigureChecker(src);  // move function to actually do it (so i can move the long switch
        boolean colordst = colorfigureChecker(dst);   // case to the bottom
        boolean colorsrc = colorfigureChecker(src);
        if (!src.checkEmpty()) {
            moveFigure(colordst, colorsrc, figurename, src, dst);
        }
        else System.out.println("Can't move nothing, son");
    }

    public boolean colorfigureChecker (Field a) {   //returns color of field a
        return a.getColor(a);
    }

    public String StringfigureChecker (Field a) {  // returns figure name of     field a
       return a.getFigure(a);
    }

    public void moveFigure(boolean colordst, boolean colorsrc, String figure, Field dst, Field src) {
        switch (figure) {
            case "Rookie":
                //if (colorsrc) - implement later to differentiate between     white and black movements (forward only)
                if ((dst.getY() - src.getY()) > 2) {
                    System.out.println("Rookie can't move more than two steps, ever.");
                } else if (dst.getX() - src.getX() > 1) {
                    System.out.println("Rookie can't move more than one step     to the side, ever.");
                } else if (dst.getX() - src.getX() == 1 && dst.getX() -     src.getX() == 1 && (colordst != colorsrc) && !fieldObject.checkEmpty(dst)) {     //fieldObject or dst.checkempty?
                    fieldObject.conquer(src, dst);
                    System.out.printf("Successfully beaten %s and moved to %c%d", StringfigureChecker(dst), dst.getX(), dst.getY());
                } else if (((dst.getX() - src.getX()) == 0) && ((dst.getY() - src.getY()) == 1)) {   //for the sake of testing, rework later for proper +1, +2 movements
                    fieldObject.conquer(src, dst);
                    System.out.printf("Successfully moved to %c%d",     dst.getX(), dst.getY());
                } else System.out.println("notpossibleblablatest"); //just to     check if basic movements work, complete latere
                break;


            default:
                System.out.println("defaultcase");
                break;
        }
    }
}

Then here's my Field class (left out some getters and setters to make it less long):

public class Field {


    private char X;
    private int Y;
    private String figure;
    private boolean color;  // true = white
    private boolean empty;  // true = field is empty

    public Field (char X, int Y) {
        this.X = X;
        this.Y = Y;
    }
    public String getFigure(Field a) {
        return a.figure;
    }

    public boolean getColor(Field a) {
        if (a.color) return true;
        else return false;
    }
    public boolean checkEmpty(Field a) {
        if (a.empty) return true;
        else return false;
    }
    public void makeEmpty (Field a) {
        a.figure = null;
        a.empty = true;
    }
    public void conquer(Field src, Field dst) {
        dst.figure = src.figure;
        dst.color = src.color;
    }
}

Main (only building an array of Fields so far to create the playfield):

public class Main {

    public static void main(String[] args) {


        char fieldChar = 'a';
        int fieldNumber = 0;

        Field[][] playfield = new Field[8][8];   //creating 8x8 playfield
        playfield[0][0] = new Field('a', 0);
        int j = 0;
        for (char i = 0; i < 8; i++){
            fieldChar = (char) (i + 65);
            for (j = 0; j < 8; j++) {
                playfield[i][j] = new Field(fieldChar, fieldNumber + j +1);
            }
        }
}

I'm not looking as much for very technical optimizations that I might not understand yet, but would appreciate advice on how to improve the structure and how to be smarter about the coding. I'd like to take every possible hint I can to learn as much I can from my mistakes or too long winded attempts of coding certain things.

I'd also like to know if it would be at all useful to use an interface or to split things into more classes to program this Chess game that I try to build.

I also realise I don't really know when and how to best use the constructor. I think it's completely unneeded for me to use my constructor to initialize my Fields. Could it not have just been done in the main loop?


Future Plans

As of now it's only these 3 classes and I plan on making a big switch case for the different figures in Chess to:

  • check if the requested movement is possible + allowed
  • maybe add a 4th class to check the game state after every move
  • see if there's checkmate, check or castling is possible
  • maybe add a few algorithms that save the current game states, so that in the next states not everything has to be recalculated (but most of that after I have a completely functioning program already)
\$\endgroup\$
  • 1
    \$\begingroup\$ Note: your "Rookie" is not a rook as one would expect, but appears to act like a pawn. \$\endgroup\$ – 200_success Aug 3 '16 at 22:30
  • 1
    \$\begingroup\$ It's hard to review code when it is full of caveats, such as being excerpted and unfinished. Please make your post more concrete. \$\endgroup\$ – 200_success Aug 3 '16 at 22:33
  • 1
    \$\begingroup\$ @DaeVo are you open to getting all aspects of your code reviewed, including but not limited to the design/structure? At the moment it is not completely clear. \$\endgroup\$ – Phrancis Aug 3 '16 at 22:46
  • 1
    \$\begingroup\$ @DaeVo I've made a pretty substantive edit to the non-code parts of your post to hopefully clarify it a bit, please review it - if you don't like it we can roll it back. In any case, welcome to Code Review! \$\endgroup\$ – Phrancis Aug 3 '16 at 23:01
  • 1
    \$\begingroup\$ You comment your functions in a very weird way. See en.wikipedia.org/wiki/Javadoc for the best practice on commenting on a function. \$\endgroup\$ – Thijs Riezebeek Aug 3 '16 at 23:45
4
\$\begingroup\$

Rather than a single class for Figures, you should create a class for each "figure" (typically known as "pieces"). This class should implement an interface of all the members that all the pieces should know about collectively (see this SO answer for an idea on how to implement Coordinate):

interface IPiece {
    boolean isValidMove(Coordinate current, Coordinate to);
    Color getPieceColor();
}

Then implement all your piece classes:

public class Rook implements IPiece {
    public Rook(Color color) {
        // set things up here
    }

    boolean IsValidMove(Coordinate current, Coordinate to) {
        // check that the coordinates are not the same

        // check that the move does not go off the board,
        // whatever coordinate you are using for the edge of the board
        // you may wish to expose this as a static value of your `Field` class

        // check that the move is valid--for example,a rook can only move in straight lines
        // this means that either the coordinate `X` or `Y` values will match
    }

    public Color getPieceColor() {
        return _pieceColor;
    }
}

As for the main program, I would have a Board (or Field) class to represent the state of the board, a set of enums for representing different things, such as piece color, and a main controller class to start the program and keep track of the state. Your controller class will keep an instance of the board, track who's turn it is, and so on.

By the time you are done, you will most definitely need more than four classes, and you should use interfaces to show how classes are related (and a class can implement multiple interfaces). An important note when developing a project like this--you should research unit testing and unit test everything.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your suggestions! I'll try to work with them tomorrow and alter my code. Why is it better to have an Interface and have the method implemented to all the classses that need it, rather than just accessing the method from outside, though? (that's part of my lacking understanding of why and how I need interfaces.. eventhough it seems to be easier to keep an eye on what methods your classes want to use.) Do you recommend using unit testing for simple fun-programs? Or is it more of a "getting used to it" thing for bigger projects? \$\endgroup\$ – DaeVo Aug 4 '16 at 0:12
  • \$\begingroup\$ It is essentially A) guaranteeing the method will be there (compilation error if it isn't), and B) making it easily accessible to the calling code, which only has to know the interface type. Suppose you have a list of IPieces, you can still call isValidMove without knowing whether the piece is a rook, king, or what. \$\endgroup\$ – Hosch250 Aug 4 '16 at 0:25
  • \$\begingroup\$ Also, if you ever have to Mock the interface (you just might if you implement unit tests), it will make it 100 times easier to mock the interface rather than an actual type. \$\endgroup\$ – Hosch250 Aug 4 '16 at 0: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.