DRY attack and defense

Question

I have these two methods on a class that differ only in one method call. Obviously, this is very un-DRY, especially as both use the same formula. Could anyone give me advice on how to tidy this up?

public class PlayerCharacter {
    int level;
    public int getAttack() {
        int attack = 1 + level;
        for(Equipment e : equipments)
           attack += e.getAttack();
        return attack * Math.sqrt(level);
    }
    public int getDefense() {
        int attack = 1 + level;
        for(Equipment e : equipments)
           attack += e.getDefense();
        return attack * Math.sqrt(level);
    }
}

I thought of refactoring the loop to an external method but the different method calls has me stumped.

Answer 1

A few things, bad messages first…

Double and Int

return Math.sqrt(level);

Math.sqrt returns a double. That is a floating point number, probably something like: \$2.352561\$. If you now "cast" that to an int (because your method says, "I only return integers"), you just cut everything behind the dot.

This can sometimes lead to funny behavior:

\$2.7371 \mapsto 2\$
\$2.0125 \mapsto 2\$

That's not nice…

Braces:

It's usually a very good idea to place braces, even when you could leave them out:

for (Equipment e : equipments) {
    attack += e.getAttack();
}

This is because if you want to do something else, too you need to add braces either way, and If you forget it, the results become "unpredictable"

Copy/Paste:

You should never do that, because as soon as you do, you usually can extract a method for it. But if you do it either way then at least take the bother to change the names…

public int getDefense() {
     int attack = 1 + level;

'nuff said on the bad stuff ;)

Attributes

You might want to consider changing your attribute structure to be more "generic". Consider the following code:

for (Equipment e : equipments) {
    defense += e.getAttribute(Attribute.DEFENSE);
}

or similarly for attack:

for (Equipment e : equipments) {
    attack += e.getAttribute(Attribute.ATTACK);
}

now we already can see, that if you do that twice, this wants to be a single method:

public int getPlayerAttribute(Attribute attribute) {
    int attribute = 1 + level;
    for (Equipment e : equipments) {
        attribute += e.getAttribute(attribute);
    }
    return attribute * Math.sqrt(level);
}

This would of course need a relatively large change in your code. You'd then need to change your Equipment from (wild conjecture):

public class Equipment {
    private int attack;
    private int defense;
    //getters and setters
}

to:

public class Equipment {
    private Map<Attribute, Integer> stats = new HashMap<>(); 
    // or a different Map implementation of your choice, like
    // EnumMap, as suggested by @ratchet freak in a comment:
    private Map<Attribute, Integer> stats = new EnumMap<>(Attribute.class);

    public int getAttribute(Attribute attribute) {
        return stats.get(attribute);
    }
}

And create a new enum to hold all the different kinds of attributes you have:

public enum Attribute {
    ATTACK, DEFENSE, HITPOINTS, WHATEVER
}

Answer 2

Before talking about refactoring, let us first talk about your code style:

1. Your level is currently a package-private variable, it is better off just being a private variable.
2. Your could could use some extra room to breathe, consider adding blank lines between the method calls.
3. Your copy-pasted getDefense is a real copy paste, you should really change the attack to defense there.
4. Always use braces around the relevant control structures! This means that your for loop needs to be along the lines of for() { ... }.
5. You should use descriptive variable names, I know it is tempting to write Equipment e, but I would really advice you to just write Equipment equipment, such that all code is much more readable.

To illustrate, I have decided to reformat the class to how I would've done it and I think it looks way better now.

public class PlayerCharacter {
    private int level;

    ...

    public int getAttack() {
        int attack = 1 + level;
        for (Equipment equipment : equipments) {
           attack += equipment.getAttack();
        }
        return attack * Math.sqrt(level);
    }

    public int getDefense() {
        int defense = 1 + level;
        for (Equipment equipment : equipments) {
           defense += equipment.getDefense();
        }
        return defense * Math.sqrt(level);
    }
}

Now we should refactor the methods to not lead to duplicatd code. I do not know which Java version you are using, but considering this seems like a toy project, I am assuming it is safe to introduce you to Java 8, which will make your life considerably easier.

Strangely I have just noticed that return attack * Math.sqrt(attack) does not compile on Java 8 (not sure if it did on your version, I doubt it), there are two ways around this:

1. Return a double from getAttack(), since Math.sqrt(level) returns a double.
2. Cast Math.sqrt(level) to an int, truncating the decimal values.

I'll go with the second option as this is the most straight forward one, but you should really look into this.

Our first revised code will look like this:

public class PlayerCharacter {
    private int level;

    ...

    public int getAttack() {
        return getAttribute(Equipment::getAttack);
    }

    public int getDefense() {
        return getAttribute(Equipment::getDefense);
    }

    public int getAttribute(final ToIntFunction<Equipment> attributeGetter) {
        int attribute = 1 + level;
        for (Equipment equipment : equipments) {
            attribute += attributeGetter.applyAsInt(equipment);
        }
        return attribute * (int)Math.sqrt(level);
    }
}

What I did here, was passing in an ToIntFunction<Equpment>, which really is nothing more than a function that converts an Equipment to an int, a compatible signature would be any lambda of the form Equipment -> int, which is where I used a method reference like Equipment::getAttack, which is in its turn a shorthand for equipment -> equipment.getAttack(), which is a shorthand for (Equipment equipment) -> equipment.getAttack().

Note however that you now cannot change the calculations for the attack respectively defense attribute anymore, hence that would need to be configurable as well in some way.

I'll update the answer when I have found a way to do so.

Answer 3

Honestly, I'm not sure I'd bother trying to merge these methods. Yes, they're essentially identical, but when you look more closely, the duplicated code is still just a few lines, most of which is unavoidable boilerplate code imposed by the language and class design you're using.

Generally, when thinking about whether to DRY up some (seemingly) repetitive code, there's a couple of questions that I'd recommend asking yourself first:

1. Is it really true duplication, or merely incidental similarity? That is, if you were to change one of the methods, would you expect having to change the other one too?

   This seems like a particularly relevant question for your example, because most of the apparent duplication comes from the fact that the attack and defense formulas are so similar. If they're similar by design, so that changing one would always require changing the other one too, then it's true duplication and might call for refactoring. On the other hand, if you can foresee the possibility that you might want to make the formulas different in the future, then combining them now may, in fact, turn out to be detrimental later.

2. Will combining the methods make the code shorter and simpler? If eliminating the duplication makes the code longer or harder to understand, then you haven't really gained anything, and may have just made things worse.

   The real purpose of all these "rules of thumb" like DRY is to save you time and to keep your code simple, efficient and maintainable. If you find yourself wasting significant development time or introducing lots of extra complexity just to follow a rule of thumb, you may be taking the rule too far.

3. Will there be more duplication like this? This is really a corollary to the previous question. If you know that there will only ever be two methods like this, and they're unlikely to become more complex than they are now, then de-duplicating them (at least for now) may be a waste of effort. However, if you think you might need a third or a fourth copy, it might be time to think about combining them into one method.

   Sometimes, if you expect a piece of code to acquire more uses in the future, it may even be worthwhile to make it flexible to enough to allow such uses without duplication, even if it hasn't been duplicated yet. Don't go too far with this, though; always keep YAGNI in mind, too.

All that said, if I really believed the similarity between the two methods to be fundamental and not just incidental, and if I really wanted to eliminate the redundancy, what I'd do (at least to start with) would be to factor the shared (1 + level + sum(equipment)) * sqrt(level) formula into a separate (static) method, leaving me with:

public class PlayerCharacter {
    private int level;

    public int getAttack() {
        int attackBonus = 0;
        for (Equipment e : equipments) attackBonus += e.getAttack();
        return powerFormula(level, attackBonus);
    }
    public int getDefense() {
        int defenseBonus = 0;
        for (Equipment e : equipments) defenseBonus += e.getDefense();
        return powerFormula(level, defenseBonus);
    }

    private static int powerFormula(int level, int bonus) {
        return (1 + level + bonus) * (int) Math.sqrt(level);
    }
}

(The choice of making the powerFormula() method static is mainly a stylistic one, but I felt it appropriate to emphasize the fact that it just encapsulates a simple mathematical formula, which exists independently of any particular character object.)

Now, there's still quite a bit of apparent repetition left, but it's really all just boilerplate for summing the attack / defense bonuses of the player's equipment. In Java 8, we could tidy up the remaining duplication by using method references, e.g. like this:

public class PlayerCharacter {
    private int level;

    public int getAttack() {
        int attackBonus = getEquipmentBonus(Equipment::getAttack);
        return powerFormula(level, attackBonus);
    }
    public int getDefense() {
        int defenseBonus = getEquipmentBonus(Equipment::getDefense);
        return powerFormula(level, defenseBonus);
    }

    private int getEquipmentBonus(ToIntFunction<Equipment> func) {
        return equipments.stream().mapToInt(func).sum();
    }
    private static int powerFormula(int level, int bonus) {
        return (1 + level + bonus) * (int) Math.sqrt(level);
    }
}

(I hope this code is correct, because I don't actually have a Java 8 compiler to test it with. It could be shortened further by combining the getEquipmentBonus() and powerFormula() methods into a single method, but keeping them separate feels cleaner to me — that way, each method has a single clearly defined responsibility. For the getEquipmentBonus() method, I chose to use the Stream interface, but you could equally well just use a simple loop.)

For earlier Java versions, the duplicate loops seem pretty much unavoidable, unless you feel like playing with inner classes to simulate Java 8 lambda expressions or something similar.

In any case, both of the refactored versions above achieve the goal of ensuring that, if you ever want to change the player attack / defense power formula, you can change it in only one place and the change will affect both calculations. If this is not what you want, you may be better off keeping your original code as it is.

Answer 4

Examining the similarities and differences I would suggest you factor out the Attack/Defence attribute into a separate class. This can then be used as a parameter. An enum is an excellent candidate for the attribute - I will call it a Stance, I am sure you will come up with a better name.

The result is very succinct and I think quite neat:

enum Stance {

    Attack {
                @Override
                int getStrength(Equipment equipmemt) {
                    // When on attack - deliver the attackng strength of the equipment.
                    return equipmemt.getAttack();
                }

            },
    Defend {
                @Override
                int getStrength(Equipment equipmemt) {
                    // When in defence - deliver the defensive strength of the equipment.
                    return equipmemt.getDefence();
                }

            };

    // All stances must delver a value depending on the equipment.
    abstract int getStrength(Equipment equipmemt);

    // Convenience method for collections of Equipment.
    int getStrength(Iterable<Equipment> es) {
        int total = 0;
        for (Equipment e : es) {
            total += getStrength(e);
        }
        return total;
    }
}

public class PlayerCharacter {

    int level;

    public double getStrength(Stance stance) {
        // Sum all strengths of all equipment in that stance.
        int strength = 1 + level + stance.getStrength(equipments);
        // Work out the value.
        return strength + Math.sqrt(level);
    }

    public double getAttack() {
        // It's the attacking strength.
        return getStrength(Stance.Attack);
    }

    public double getDefense() {
        // It's the defending strength.
        return getStrength(Stance.Defend);
    }
}