Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm writing a small text-based game in Java to learn the language. I'm concerned that I may be making some poor design decisions. I'll introduce 2 elements: A character and monsters. A singleton character should be able to fight a wide array of monsters.

Here is the monster class:

public abstract class Monster {

    protected static Random random = new Random();
    protected static int STATS_PER_LEVEL = 3;


    public int combatLvl;
    public int defense, strength, attack, hp;
    protected String name;

    protected Monster(int defense, int strength, int attack, int hp) {
        this.defense = defense;
        this.strength = strength;
        this.attack = attack;
        this.hp = hp;
        this.combatLvl = (int) (defense+strength+attack+Math.round((double)hp/10)-4)/STATS_PER_LEVEL;
    }

    public int attack() {
        //removed the body
    }

    public String toString(String name) {
        return this.name;
    }
}

Here's a subclass:

public class Bandit extends Monster {

    public static double averageLevelDouble = -1;
    public static String averageLevelString = "dummy";

    public static int minDefense = 1; 
    public static int maxDefense = 1;
    public static int minStrength = 1;
    public static int maxStrength = 3;
    public static int minAttack = 1;
    public static int maxAttack = 3;
    public static int maxHp = 4;

    public Bandit() {
        super(random.nextInt(maxDefense-minDefense+1) + minDefense, 
              random.nextInt(maxStrength-minStrength+1) + minStrength, 
              random.nextInt(maxAttack-minAttack+1) + minAttack,
              random.nextInt(maxHp-minHp+1) + minHp);
        double min = minDefense+minStrength+minAttack+((double)minHp/10);
        double max = maxDefense+maxStrength+maxAttack+((double)maxHp/10);
        double avg = ((max+min)/2)-4;
        averageLevelDouble = (double)Math.round((avg/STATS_PER_LEVEL)*10)/10;
        averageLevelString = (Math.round(min)-4)/STATS_PER_LEVEL + "-" + 
                     (Math.round(max)-4)/STATS_PER_LEVEL;
        name = "Bandit";
    }   
}

My main concern is that I should be able to easily expand the game with arbitrarily many monsters. The way I'd do it now is to copy paste Bandit into a new class: Change the class name to f.i. Skeleton, initiate 'name' variable in the constructor, customize all the static ints. Is this a wrong approach?

Also a very obvious thing that comes to mind is that I should be able to display information on a particular monster without having to instantiate it. From a Swing.JComboBox I should be able to choose all monsters, display their information, and at the moment I decide to fight a monster it should be instantiated. I can't find a solution to represent monsters on a JList or JComboBox without having to manually add Bandit.name (pretend it's static) as a line of code in the JList/JComboBox class when I define the Bandit class.

I'm just assuming that I shouldn't be instantiating my subclasses. Again, that is the question: What am I doing wrong and right? I really don't how to ask but; What's wrong with my code?

share|improve this question

3 Answers 3

up vote 7 down vote accepted

Your Monster class should have very few, even no static fields. All static fields, if any, should be constants only ( private static final ....).

What you want to do is separate the definition of a monster from it's implementation. Also, the implementation of the monster should be based on a template that sets it's characteristics. The Definition can be that template too.

So, consider a class that is something like:

public class MonsterDefinition {
    private String monsterType;
    private double maxDefense
    .....

    public MonsterDefinition(.......) {
        .. set up the definition
    }

    // getters for the templates.

}

Then, you will need one instance of the MonsterDefinition for each of your monster types. This is what you will use in your ListViews, etc. They will be things like:

private static final MonsterDefintion[] MONSTERS = {
     new MonsterDefintion("Skeleton", .....),
     new MonsterDefintion("Bandit", .....),
     new MonsterDefintion("CodeGolf", .....),
     new MonsterDefintion("Zombie", .....),
     ......
 }

Then, you can have a class called something like Monster... which will take a definition to initialize itself:

public class Monster {

    private final MonsterDefinition definition;
    private final int id;
    private final String name;

    public Monster(String name, int id, MonsterDefinition definition) {

        this.definition = definition;
        this.name = name;
        this.id = id;
    }

    ......


    public String toString() {
        return String.format("I am monster %d with name %s of type %s, id, name, definition.getMonsterType());

    }
}
share|improve this answer
    
"you will need one instance of the MonsterDefinition for each of your monster types". You mean one MonsterDefinition for each monster, right? So MonsterDefinition will take something like 10+ arguments? Monster will store the MONSTERS variable? So MonsterDefinition should include a monster's name, as their id for the JList? Could you read your answer over to see if you have made any errors, there seems to be inconsistency. I hope that's what it is and not me understand it wrong. –  user2651804 Mar 19 at 0:29
1  
I mean 1 monsterdeginition for each monster type, 1 definition for 'zombie', 1 definition for 'skeleton', etc. Then you use that to make a 'copy', like a template for each Monster. –  rolfl Mar 19 at 0:30
    
I don't think I quite understand. My only reference to Skeleton would be MONSTERS[0], right? Is that what you call a template? It means that all skeletons would be exactly identical; But they should have some randomness in stat-generation from instance to instance. –  user2651804 Mar 19 at 0:38
    
The reason I asked if you meant one MonsterDefinition for each monster is that you seem to differentiate between monstertype and monstername –  user2651804 Mar 19 at 0:41
    
Oh okay, I see I wouldn't have that problem since it's the Monster class I want to instantiate using MonsterDefinition. I still have some questions, but this answer really helps me a lot and achieves what I want, thanks! –  user2651804 Mar 19 at 0:49

You don't need a separate class for it!

I don't see any real difference between a Bandit and a Skeleton and a regular monster. You don't add or change any methods in the subclasses, and you don't add any new variables (other than the static ones).

The most important change you can make here is to only use one class. And then you'd want different methods to instanciate that class. This can be considered factory methods

A current smell of your code is that your call to the super constructor is a very long one and it's using many variables and some calculations on them. Factory methods will deal with this.

I also think that you are overusing static variables. There are a bunch of ways to deal with this but in this example I will just put them as local variables in a method. A future extension would be to create a MonsterConfiguration class that keeps these variables as fields, then you can create several MonsterConfigurations, and perhaps read some MonsterConfigurations from XML?

In your Monster class, add String name as a parameter to the constructor and remove the abstract keyword from it.

public Monster createBandit() {
    int minDefense = 1; 
    int maxDefense = 1;
    int minStrength = 1;
    int maxStrength = 3;
    int minAttack = 1;
    int maxAttack = 3;
    int maxHp = 4;

    int defense = random.nextInt(maxDefense-minDefense+1) + minDefense;
    int strength = random.nextInt(maxStrength-minStrength+1) + minStrength;
    int attack = random.nextInt(maxAttack-minAttack+1) + minAttack;
    int hp = random.nextInt(maxHp-minHp+1) + minHp;
    return new Monster("Bandit", defense, strength, attack, hp);
}

I would suggest moving these things to fields in the class or perhaps preferably, methods of the Monster class:

    double min = minDefense+minStrength+minAttack+((double)minHp/10);
    double max = maxDefense+maxStrength+maxAttack+((double)maxHp/10);
    double avg = ((max+min)/2)-4;
    averageLevelDouble = (double)Math.round((avg/STATS_PER_LEVEL)*10)/10;
    averageLevelString = (Math.round(min)-4)/STATS_PER_LEVEL + "-" + 
                 (Math.round(max)-4)/STATS_PER_LEVEL;

These methods could be called something like: getMin, getMax, getAvg, getAverageLevelDouble, getAverageLevelString.

Now if you want to have a Skeleton, just make a createSkeleton method.

share|improve this answer
    
I almost don't understand anything from your post other than -> Don't make a subclass if it doesn't add functionality. I don't at all understand how putting the calculations of averageLevel(Bandit) outside the creating method would work? I'm not very happy about the duplicate create methods either and I'll still need call createNewMonster on my JList class every time I define a new monster. Have you read rolfl's answer below? It seems very solid, I'm going to try implementing it tomorrow. After reading his post, is there anything you would do different than him? –  user2651804 Mar 19 at 1:29
    
@user2651804 His answer is just a further extension of what I'm doing here. His MonsterDefinition is exactly the MonsterConfiguration I mentioned. I also don't know how different your monsters will be, if they all match the pattern then stick to rolfl's answer. I honestly don't know what you plan on using your averageLevel variables for, so I can't answer you properly there. I don't know your skill level, but I suggest that you learn about the Factory pattern and the proper Java terminology (especially what a field is). Once you've improved your code, post a follow-up question. –  Simon André Forsberg Mar 19 at 11:12

You have four similar structures here:

super(random.nextInt(maxDefense-minDefense+1) + minDefense, 
          random.nextInt(maxStrength-minStrength+1) + minStrength, 
          random.nextInt(maxAttack-minAttack+1) + minAttack,
          random.nextInt(maxHp-minHp+1) + minHp);

Creating a randomInt(int lower, int upper) method (or using RandomGenerator.nextInt(int lower, int upper) from Apache Commons Math) would improve readability of the code and remove some duplication:

super(randomInt(minDefense, maxDefense), 
    randomInt(minStrength, maxStrength), 
    randomInt(minAttack, maxAttack),
    randomInt(minHp, maxHp));
share|improve this answer
    
I've never used any library that wasn't prefixed with java: import java..., the oracle library I suppose? how can my code import RandomGenerator? –  user2651804 Mar 19 at 11:30
    
@user2651804: You can download it from commons.apache.org/proper/commons-math/download_math.cgi and you can add the jar to your classpath. The exact way mainly depends on your development environment. Are you using Eclipse, Netbeans or something similar? What is your build tool? Is it plain javac or something else? –  palacsint Mar 19 at 12:08
    
I'm using Eclipse, so that renders the question of my build tool mood, doesn't it? I'm still curious what the difference would be. If you could point me towards an article explaining it I would love that –  user2651804 Mar 19 at 13:17
    
@user2651804: I guess you need that: stackoverflow.com/questions/179024/… (For example, if you were using Maven, you would need to add a new dependency XML tag to its pom.xml configuration file.) –  palacsint Mar 19 at 14:18

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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