5
\$\begingroup\$

I am coding a 2D board game. I let the user choose the difficulty level in the beginning, which influences the skills points of the player.

Here is the code I wrote for the Player class:

package model;

public class Player {

    // VARIABLES ---------------------------

    private Position position;
    private int score = 0;
    private int stepsLeft;
    private int fightingSkill;
    private int jokingSkill;
    private int visionScope = 2;
    private String skillChoice;


    // CONSTRUCTOR --------------------------

    public Player(Position position, int difficultyLevel) {
        this.position = position;

        switch(difficultyLevel) {
        case 1: 
            this.stepsLeft = 150; 
            this.fightingSkill = 5; 
            this.jokingSkill = 5;

        case 2: 
            this.stepsLeft = 150; 
            this.fightingSkill = 2; 
            this.jokingSkill = 2;

        case 3: 
            this.stepsLeft = 100; 
            this.fightingSkill = 2; 
            this.jokingSkill = 2;

        case 4: 
            this.stepsLeft = 10; 
            this.fightingSkill = 1; 
            this.jokingSkill = 1;
        }
    }


    public Player(int stepsLeft, int fightingSkill, int jokingSkill) {
        this.stepsLeft = stepsLeft; 
        this.fightingSkill = fightingSkill; 
        this.jokingSkill = jokingSkill;
        this.position = new Position(1,1);
        this.score = 0;
    }

    // METHODS ------------------------------

    public void move(Position destination) {
        setPosition(destination);
        stepsLeft -= 1;
    }

    public void increaseScore(int bonus) {
        score += bonus;
    }

    public void increaseStepsLeft(int bonus) {
        stepsLeft+= bonus;
    }

    public void increaseFightingSkill(int bonus) {
        fightingSkill += bonus;
    }

    public void increaseJokingSkill(int bonus) {
        jokingSkill += bonus;
    }

    // GETTERS 

    public Position getPosition() {
        return position;
    }

    public int getXPosition() {
        return position.getX();
    }

    public int getYPosition() {
        return position.getY();
    }

    public int getScore() {
        return score;
    }

    public int getStepsLeft() {
        return stepsLeft;
    }

    public int getFightingSkill() {
        return fightingSkill;
    }

    public int getJokingSkill() {
        return jokingSkill;
    }

    public String getSkillChoice() {
        return skillChoice;
    }

    public int getVisionScope() {
        return visionScope;
    }

    // SETTERS

    public void setPosition(Position position) {
        this.position = position;
    }

    public void setSkillChoice(int choice) {
        switch(choice) {
        case 0:
            this.skillChoice = "joke";
            break;
        case 1:
            this.skillChoice = "fight";
            break;
        case 2:
            this.skillChoice = "magic";
            break;
        default:
            ;
        }
    }

    public String toString() {
        return "play \t"+position+"\t"+stepsLeft+"\t"+jokingSkill+"\t"+score+"\n";
    }
}

My questions

  1. I chose to use a switch statement in the constructor, to create my player with features according to the difficulty level. I have the feeling that is maybe not the good practice to have such a switch in a constructor but can't really say why, and don't find a better way. Is there a better way to do this?

  2. Any other comments about that class? I've programmed scientific code but am quite new to OOP and Java. Any comment is welcome.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ If the files will fit in a single question, then go ahead. If not, you can post a subset of the files (preferably of related functionality) here and link offsite to the rest. Repeat with different subsets (which may overlap) until you get reviews on everything. Note: if this will create more than four or five reviews, it may be too much. More discussion. \$\endgroup\$
    – Brythan
    Commented Jan 1, 2015 at 16:25
  • 1
    \$\begingroup\$ Since I don't think anyone else mentioned it, and it isn't a big enough issue to warrant an answer, in your original code, you're missing some 'break;' statements in your constructor's switch statement. That'll cause something called "fallthrough". No matter what difficulty is selected, 'stepsLeft' will always be 10, and 'fightingSkill' and 'jokingSkill' will both always be 1. \$\endgroup\$
    – Gabe Evans
    Commented Jan 2, 2015 at 15:08

2 Answers 2

4
\$\begingroup\$

There are several things that can be improved, in my opinion:

  1. Whitespaces around operators make the code more readable (you use them almost everywhere except this method):

    public String toString() {
        return "play \t" + position + "\t" + stepsLeft + "\t"
               + jokingSkill + " \t" + score + "\n";
    } 
    
  2. Comments like // VARIABLES ---------- or // METHODS ---------- are useless (they do not contain any additional information). Adding comments for each individual method explaining what it does (especially for public ones) or what a field is used for is a good practice.

  3. An alternative for using switch statement is creating an array of constants. It can look like this:

    private static int[] stepsLeftForLevel = new int[]{150, 150, 100, 10};
    
    ...
    
    public Player(Position position, int difficultyLevel) {
        ...
        stepsLeft = stepsLeftForLevel[difficultyLevel];
        ...
    }
    

    It makes the code more readable in my opinion because all "magic" numbers are located in one place (and the code becomes more concise).

  4. There is no need to use this in a constructor unless the name of the field is shadowed by a parameter (that is, I would recommend using this only when it is necessary).

\$\endgroup\$
3
\$\begingroup\$

There are a few things I can talk about here.

First, I would suggest using an enum instead of integers for the switch statement. So this:

    switch(difficultyLevel) {
    case 1: 
        this.stepsLeft = 150; 
        this.fightingSkill = 5; 
        this.jokingSkill = 5;

    case 2: 
        this.stepsLeft = 150; 
        this.fightingSkill = 2; 
        this.jokingSkill = 2;

Would end up looking more like this:

public enum DifficultyLevel {
    EASY, MEDIUM, HARD, IMPOSSIBLE
}

switch(DifficultyLevel) {
    case EASY: 
        this.stepsLeft = 150; 
        this.fightingSkill = 5; 
        this.jokingSkill = 5;

    case MEDIUM: 
        this.stepsLeft = 150; 
        this.fightingSkill = 2; 
        this.jokingSkill = 2;

My syntax may not be perfect here, but this approach is definitely more readable. Also I should note that Java allows you to use strings with switch statements, so you can set it up that way also for the same effect.


I don't understand why you have multiple constructors here:

public Player(Position position, int difficultyLevel) {
    this.position = position;

    switch(difficultyLevel) {
    case 1: 
        this.stepsLeft = 150; 
        this.fightingSkill = 5; 
        this.jokingSkill = 5;

    case 2: 
        this.stepsLeft = 150; 
        this.fightingSkill = 2; 
        this.jokingSkill = 2;

    case 3: 
        this.stepsLeft = 100; 
        this.fightingSkill = 2; 
        this.jokingSkill = 2;

    case 4: 
        this.stepsLeft = 10; 
        this.fightingSkill = 1; 
        this.jokingSkill = 1;
    }
}

public Player(int stepsLeft, int fightingSkill, int jokingSkill) {
    this.stepsLeft = stepsLeft; 
    this.fightingSkill = fightingSkill; 
    this.jokingSkill = jokingSkill;
    this.position = new Position(1,1);
    this.score = 0;
}

Are you using both constructors in different places? Even if you are doing so, then it should still be possible to combine both constructors into one, or call the second constructor from inside the first one. The code is confusing because the first constructor sets the fields directly, but doesn't set the score, and the second constructor sets the position directly.


The argument names here are a bit confusing:

public void increaseStepsLeft(int bonus) {
public void increaseJokingSkill(int bonus) {

The word "bonus" implies something about the meaning of the increases, but this seems unnecessary to me, and potentially confusing. Why is it a bonus? I would probably just say (int amount) since the method increases the variable by the value provided.


Is it really necessary to have separate getters for the X and Y coordinates?

public int getXPosition() {
    return position.getX();
}

public int getYPosition() {
    return position.getY();
}

You already have another getter for the entire Position, so why not just use that? The usage would look like this:

player.getPosition().getY();

You may want to consider some high level comments for the Player class that explain its intended usage. This is not always necessary for all classes, but leaving come high level comments for yourself may help you to more easily understand the code when you come back to it later. To be clear, I am talking about something like this:

package model;

/*
    The player selects their primary skill at the start of the game.  
    The choice of skill increases the bonus amounts that the player receives for that skill. etc etc
*/

public class Player {
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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