Skip to main content
edited body
Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 160
  • 312

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 thingthink 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.

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 thing 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.

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.

Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 160
  • 312

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 thing 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.