Question Details

No question body available.

Tags

object-oriented code-smell

Answers (5)

Accepted Answer Available
Accepted Answer
March 28, 2025 Score: 45 Rep: 221,438 Quality: Expert Completeness: 60%

It seems you missed the point of the recommendation against instanceof.

instanceof is not the problem, it is only a symptom - a symptom of not using polymorphism where it would be better to use it. When you replace instanceof by a fancy (and btw. quite inefficient) use of System.identityHashCode, you are still missing out on using polymorphism.

Note in your contrived example, using polymorphism instead of instanceof seems to be easy: for example, add a method getStats to the interface Tool which returns all the three stat values atk, def, and speed in some way (maybe introducing a specific helper type for stat tuples), and implement this method in all the tool classes. Then you will need neither instanceof nor System.identityHashCode. Depending on how your real code looks like, you can design this differently - as you can see from the other answers and the comments, there is more than one option for this.

March 28, 2025 Score: 10 Rep: 140,596 Quality: High Completeness: 60%

There are several flaws in your approach.

In terms of performance, this is a disaster. There is a chance that you won't notice it in your specific case. If printToolsAndTotalStats gets executed outside any loop and if the number of tools remains small, the fact that you are looping over and over through the different types of tools may only cost you a few microseconds—nothing to worry about. But put such code in a large multiplayer video game, and things could quickly get ugly.

There is an actual bug in your code. System.identityHashCode determines a hash code of an object, and hash code collision is possible. Nothing prevents you from having two magic cloths side by side in your collection having the exact same hash code. Or a sword that shares a hash code with a shield. Now, guess, what would your app so if it takes a sword for a shield?

Essentially, your code has the very same problem than the one with instanceof, but with additional issues.

Instead, use polymorphism to get atk, def and speed metrics.

March 28, 2025 Score: 9 Rep: 12,369 Quality: High Completeness: 60%

You figured out that you can hold onto multiple references, of different parts of a type hierachy, but then kept thinking you had to deal will all the tools in one go.

Rather than figure out a way to identify if a particular tool is a Sword, you can instead loop over tools to do Tool-specific code in a tool loop, Sword-specific code in a sword loop, etc.

This does mean that you are doing some of the calculations in a different order, which is fine for addition, but makes a difference for e.g. text output.

public void printToolsAndTotalStats(){
    int totalAtk=0;
    int totalDef=0;
    int totalSpeed=0;
    for(Tool tool : tools){
        tool.printInfo();
    }
    for(Sword sword : swords){
        totalAtk+=sword.atk;
    }
    for(Shield shield : shields){
        totalDef+=shield.def;
    }
    for(MagicCloth magicCloth : magicCloths){
        totalAtk+=magicCloth.atk;
        totalDef+=magicCloth.def;
        totalSpeed+=magicCloth.speed;
    }
    System.out.println("total atk:"+totalAtk+" , total def:"+totalDef+" , totalSpeed:"+totalSpeed);
}

The other thing that you could do, is lift the stats into Tool, making sure there was an appropriate "empty" representation. In your case they could remain int, defaulting to 0, but if they become more detailed then they would need their own type.

interface Tool {
    default Optional getAttack() { return Optional.empty(); }
    default Optional getDefence() { return Optional.empty(); }
    default Optional getSpeed() { return Optional.empty(); }
}

class Sword implements Tool { private Attack attack; public Optional getAttack() { return Optional.of(attack); } }

etc.

March 29, 2025 Score: 2 Rep: 323 Quality: Low Completeness: 40%

There are two problems with both pieces of code:

  1. They are not DRY (don't repeat yourself)
  2. The behaviour for one object is not contained in one place

Having a new line for every possible item when calculating a total is an obvious case of repetition where it is not needed. Within both implementations each attack item needs a new line to add the attack attribute. Clearly that isn't necessary, because you could easily have one summation per attribute, rather than one summation per item.

In the example you have three items and it is already verbose and hard to read. If you had 3000 items the code would be awful to read and catastrophic to maintain.

What makes this far worse is that you have defined the items in one place but defined their implementation in another. Whenever you add or remove an object you have to change the code in two different places. That's begging for a mistake as well as being a lot of hassle.

March 28, 2025 Score: 1 Rep: 16,637 Quality: Medium Completeness: 30%

So what if we want a new tool? You'd have to add another test to player. What if you want to support mods? How will player understand them? Its an anti-pattern because each step becomes progressively more difficult to make.

What you really need is for the Player class to collect the effects of the tool in each situation the Player is interested in. eg: the print situation, the stats situation, the attack situation, the defense situation...

Then the tool can describe its effect in that situation to the player. eg: here is what I'm described as, here are the stats, i do this much damage, i adjust the incoming attack like this...

To achieve this we use the Visitor pattern.

The Tool interface will gain a method per situation, and that method will accept a visitor interface. The visitor interface might be common, or it might be situation dependent, let the code guide you. The visitor will have methods on it to describe the relevant effects. The player then creates a situation object (the visitor) and passes it to the tools which then describe their effects on the situation.

For example an attack visitor might be passed to a sword which describes the base damage as 3, then to a ring of swordsmanship which says there is a 1.5 multiplier on damage, then to the cursed shoes of clumsyness that says the final output should have a chance to miss of 10% and damage should be reduced by 5 half the time. The visitor would have methods to receive base damage, multiplier, chance to miss, and conditional penalities either as a method accepting an aggregate or as several individual methods for specific parts of the calculation.

Its the visitors job to decide how the effects combine in the situation and apply them to the player.