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 am building a simple adventure game that has "upgrades" feature.
In this game, user controls one player, that can be upgraded to increase things such as:

  • Amount of gold earned
  • Maximum health
  • Attack power
  • (more of this would be added later, for example: defense upgrade, experience, etc.)

An "upgrade" has a max level cap, effects on each level of upgrade, and the cost for each upgrade.

Here's my current approach.

This is my Upgrades class, containing all possible upgrades:

public class Upgrades {

    public static final int GOLD_LEVEL_CAP = 10;
    public static final float GOLD_EFFECT_PER_LEVEL = 0.01f;
    public static final int[] GOLD_UPGRADE_COSTS = {
        100, 200, 300, 400, 500,
        600, 700, 800, 900, 1000
    };

    public static final int HEALTH_LEVEL_CAP = 10;
    public static final int HEALTH_EFFECTS_PER_LEVEL = 100;
    public static final int[] HEALTH_UPGRADE_COSTS = {
        100, 200, 300, 400, 500,
        600, 700, 800, 900, 1000
    };

    public static final int POWER_LEVEL_CAP = 5;
    public static final float POWER_EFFECT_PER_LEVEL = 0.05f;
    public static final int[] POWER_UPGRADE_COSTS = {
        1000, 2000, 3000, 4000, 5000
    };

}

And this is the class that stores the current upgrades: (owned by Player class)

public class UpgradeProgress {

    private int goldUpgradeLevel = 0;
    private int healthUpgradeLevel = 0;
    private int powerUpgradeLevel = 0;

    public void upgradeGold() {
        if (goldUpgradeLevel < Upgrades.GOLD_LEVEL_CAP) {
            goldUpgradeLevel++;
        }
    }

    // Getter
    public int getGoldUpgradeLevel() {
        return goldUpgradeLevel;
    }

    // and so on..
    // same for Health and Power
}

However, if I want to add a new upgrade, for example, I wanted to add a "Defense Upgrade". I should modify both Upgrades and UpgradeProgress classes.
Is this efficient and acceptable?
Or is there any better approach?

share|improve this question

3 Answers 3

up vote 15 down vote accepted

I don't know any Java, but I think I can give you some ideas that may help you refine this code and make it easier to extend. Take a look at my question about skill upgrades on this site, because I got some very good feedback on this and I think it is relevant to your situation.

Hard coding all of those values into the Upgrades class limits you in many ways. It is nice that everything is in in one place for the sake of easily editing it all at once, but as you note it is forcing you to change data in multiple places if you want to extend the class at all, or even if you want to add new kinds of Upgrades to the game.

What I suggest (which is also the suggestion made to me in the question I linked above) is to make a base Upgrade class (though possibly with a different name), and then make one for each of the Upgrades that you want in your game. Give it all the values that you are assigning in the Upgrades class, and give it the logic that you have in the UpgradeProgress class. This way, all of your Upgrades will have the same underlying properties and functionality, and you can create a limitless number of them without having to change anything. Just instantiate them with the values you want.

You may need a different base class for DefenseUpgrade or any other Upgrade that has different logic inside. It may or may not be a good idea to have it inherit from the original class, it just depends on how many of the properties and methods are the same. In general though, I would recommend composition over inheritance whenever possible.

This code may be wrong because I don't know Java but here is the basic idea I am suggesting (syntax improved by @Larethian):

public abstract class Upgrade {

    private int LEVEL_CAP;
    private float EFFECT_PER_LEVEL;
    private int[] UPGRADE_COSTS;

    private int currentUpgradeLevel = 0;

    protected Upgrade(int levelCap, float effectPerLevel, int[] upgradeCosts){
        LEVEL_CAP = levelCap;
        EFFECT_PER_LEVEL = effectPerLevel;
        UPGRADE_COSTS = upgradeCosts;
    }

    public void increaseUpgradeLevel() {
        if (currentUpgradeLevel < LEVEL_CAP) {
            currentUpgradeLevel++;
        }
    }

    // Getter
    public int getUpgradeLevel() {
        return currentUpgradeLevel;
    }

    public int getTotalEffect() {
        return effectPerLevel*currentUpgradeLevel;
    }

    //could have getters or setters for any properties necessary
}

The Player would either have a few of these upgrades and would call the necessary logic to increase their levels when appropriate, or the Player could have another class such as UpgradeCollection that could contain all of the Upgrades inside it.

share|improve this answer
    
use camelCase if it is not final static. –  JaDogg Aug 9 at 19:30

As bazola and I disagree on the best course of handling this, here my own answer:

Take bazolas Upgrade-class from his answer and use the following implementation to realise an upgrade.

public class DamageUpgrade extends Upgrade {
    public DamageUpgrade(int levelCap, float effectPerLevel, int[] upgradeCosts) {
        super(levelCap, effectsPerLevel, upgradeCosts);
    }
}

public class Player {
    private float damage = 10; //base value
    private DamageUpgrade dmgUp = new DamageUpgrade(2, 2, new int[]{10,20});

    public float getDamage(){
        //returns 10 with no Upgrade, 12 on first and 14 on second Upgrade
        return damage + dmgUp.getTotalEffect();
    }
}

The advantage of creating classes for every upgrade-type is in my opinion that it is easy to implement and you can see the avaible Upgrades relativly easy. This solution could be improved by using a map to hold the different Upgradetypes, which would make it easy to add a new Upgradetype on the fly at Runtime, but is in this type of game not necessary, as all Upgrades are already known at Compiletime (at least should be).

The disadvantage is of course that your workspace gets cluttered. You will have a lot of files with nearly no new content. This means you have to organise yourself, for example by using packages (which I recommend anyway).


To those who dislike multi-class solutions:

Use Enums. Forget everything from before, I will recycle names now. And levelCap is reducted as Arraysize already implies it.

public enum Upgrade{
    DAMAGE(2, new int[]{10,20}),
    RANGE(1, new int[]{5,10,15,20});

    private float effectsPerLevel;
    private int[] upgradeCosts;

    private Upgrade(float effectsPerLevel, int[] upgradeCosts){
        this.effectsPerLevel = effectsPerLevel;
        this.upgradeCosts = upgradeCosts;
    }

    //special getter for nonexistent field, because we still need the levelcap
    public int getLevelCap(){
        return upgradeCosts.length;
    }

    //add getters for all fields, in the following I assume you did this
}

Now comes the real magic. I will use an EnumMap to keep track of the current level for every upgrade. This needs some preparation. I will have to initialize every field with 0 and computing the final result is a bit more complicated. As it is a bit more complicated, upgrading is included as well. If you have questions to that answer, feel free to comment and I will clarify.

public class Player{
    private EnumMap<Upgrade, Integer> upg;
    private float damage = 10;
    private int balance;    //included to show upgrading

    //Note: You can extend this constructor as much as you want, this is the minimum
    public Player() {
        upg = new EnumMap<>(upg.class);
        for (Upgrade u : Upgrades.values()){
            upg.put(u, 0);
        }
    }

    public float getDamage(){
        return damage + upg.get(Upgrade.DAMAGE) * Upgrade.DAMAGE.getEffectPerLevel();
    }

    public void levelUp(Upgrade u){
        if(upg.get(u) >= u.getLevelCap()) return; //early escape, we can't upgrade
        if(balance < u.getUpgradeCost()[upg.get(u)]) return; //early escape, not enough money.

        balance = balance - u.getUpgradeCost()[upg.get(u)];
        upg.put(u, upg.get(u) + 1); //fetch the value, add one, store it back
    }
}

Advantages: Once setup, adding a new upgrade is as simple as adding another enum-entry (as shown with RANGE. Whenever you add a new Upgrade-Type, it will automatically have the level 0, so you dont have to worry about the Upgrade not beeing initialized. Once you got into it, using enums and maps is awesome. You can use the enum in a lot of other places as well. Want to generate a button for each upgrade but dont want to rewrite your whole GUI everytime? Use Upgrade.values() to get an array of all Upgrades, which automatically gives you the amount of upgrades.

Disadvantage: ONCE setup, it works. Getting there is complicated and needs a solid understanding of Enums and Maps (more of the former than the latter).


Conclusion: Easier is the first version, in the long term better (because better reusable) the second version. This is at least my opinion after 2 hours of thinking this through.

share|improve this answer
    
I definitely think that both approaches have their uses as well as their pros and cons. I thought your suggestions deserved their own answer so I'm glad you posted one! –  bazola Jul 31 at 1:13
    
@bazola I figured the enum answer out when I completly thought through my Multi-Class-approach. Before I could not think of a non-hacking way to do this, therefore I edited your answer with the inherited classes, as from my point of view, you were suggesting it in your answer. Because I'm curious, what single-class-approach did you have in mind? –  Larethian Jul 31 at 7:56
    
My idea was to instantiate multiple Upgrades at runtime with their names and starting values, and then either the Player manages them directly, or the Player has a Collection of the Upgrades that it manages. This works as long as the underlying logic of the Upgrades is all the same, if the logic is different more classes are needed. I like your EnumMap approach though, looks like a good way to set things up. –  bazola Jul 31 at 11:41
    
@bazola this would result in multiple anonymous classes, wouldn't it? I'm currently thinking of a totally generic way to add any upgrade with a choosen basevalue ect. at runtime using anonymus classes, which would recycle your baseclass and a Map where 1 Key maps to several (currently 2) Values. I believe this to be absolutely unsuitable for topher, but will include it probably anyway for sake of completness. Again, thanks for the idea ;) –  Larethian Jul 31 at 12:37
    
Okay, due to Map.Entry 's Constructor not beeing accessible, this would be hacking of the worst kind and as such I won't post it here. At least not until someone gives me a proper Map with 2 (/more) Values per Key. –  Larethian Jul 31 at 15:51

The current approach is not an intuitive object oriented design. The Upgrades class is basically like a struct in C, it's just a collection of "stuff" grouped together. The plural name "Upgrades" itself already suggests that this is a collection of things, and the natural way to work with a collection of "things" is to create a Thing class and put your things inside a Collection<Thing>.

Perhaps it will make sense to have a Feature class to represent features, something like this:

class Feature {
    private final String name;
    private final float effectPerLevel;
    private final int[] upgradeCosts;

    Feature(String name, float effectPerLevel, int[] upgradeCosts) {
        this.name = name;
        this.effectPerLevel = effectPerLevel;
        this.upgradeCosts = upgradeCosts;
    }

    public int getLevelCap() {
        return upgradeCosts.length;
    }
}

Based on Feature, you could create the convenience classes Gold, Health, Power:

class Gold extends Feature {
    public Gold() {
        super("Gold", .01f, new int[]{
                100, 200, 300, 400, 500,
                600, 700, 800, 900, 1000
        });
    }
}

class Health extends Feature {
    public Health() {
        super("Health", 100, new int[]{
                100, 200, 300, 400, 500,
                600, 700, 800, 900, 1000
        });
    }
}

These are convenience classes because you could create these features by calling the constructor of Feature directly. It will depend on your full program which approach will be more ergonomic.

I dropped the constant for the level cap, because (at least for now), it seems you can derive that from the upgradeCosts array field.

I did not include here the upgrading logic on purpose. One reason for that is separating responsibilities: ideally a class should have a single responsibility. The responsibility of Feature is to describe a feature, by holding its characteristics. If I want to keep track of the current level of the feature and upgrading, that would be a second responsibility.

Another reason to put the upgrading logic somewhere else is to avoid duplication when you have multiple player objects. Every player probably has one or more features. They may have those features at different levels, but the defining characteristics of the features are the same, only the levels are different. In other words, Feature objects can be shared by multiple players, and you can let players vary only in the levels. Perhaps this could be one way to capture this notion:

class PlayerFeature {
    private final Feature feature;
    private int level;

    PlayerFeature(Feature feature) {
        this.feature = feature;
        this.level = 1;
    }

    void upgrade() {
        if (level < feature.getLevelCap()) {
            ++level;
        }
    }
}

And you could define Players that can have a collection of PlayerFeatures:

class Player {
    Set<PlayerFeature> features = new HashSet<PlayerFeature>();

    public void addFeature(PlayerFeature feature) {
        features.add(feature);
    }
}

And setup a game with something like this:

class Game {
    public void initPlayers() {
        Gold gold = new Gold();
        Health health = new Health();

        Player player1 = new Player();
        player1.addFeature(new PlayerFeature(gold));
        player1.addFeature(new PlayerFeature(health));

        Player player2 = new Player();
        player2.addFeature(new PlayerFeature(gold));
        player2.addFeature(new PlayerFeature(health));
    }
}

By splitting up the original Upgrades class this way, you gain flexibility. And if you want to make some change to one of the features, for example Gold, you can easily jump to the class with keyboard shortcut in modern IDEs, whereas you would have to do a text search in the growing Upgrades class.

share|improve this answer

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.