Design pattern to avoid violating Open-Closed principle-CodePudding

I'm building a simple game in Java. I have a couple of classes, I omitted the fields that are not relevant to my problem:

public class Character {
    //stores relics and artifacts
    public Set<Collectable> inventory;

    public void collect(Collectable collectable) {
        collectable.collect(this);
    }
}

public class Artifact extends Collectable {
    @Override
    public void collect(Character character) {
        character.inventory.add(this);
    }
}

public class Relic extends Collectable {
    @Override
    public void collect(Character character) {
        character.inventory.add(this);
    }
}

public class Spell extends Collectable {
    @Override
    public void collect(Character character) {
        Wizard wizard = (Wizard) character;
        wizard.spellBook.add(this);
    }
}

public class Wizard extends Character {
    //stores spells
    public Set<Collectable> spellBook;
}

public class Warrior extends Character {
    //fields and methods ommited
}

As of right now when I'm collecting a Spell, it has to go into a Wizard's spellBook. Warriors can't collect Spells, they don't have a spellBook. If I understand correctly from an OOP POV, a Collectable has to be able to decide where it goes (inventory or spellbook) when it's collected, hence my solution above.

My problem is that I have to use typecasting in Spell.collect(Character) to be able to put the Spell into a Wizard's spellBook, because by default, spellBook is not visible on Character, and I think it shouldn't be, because then Warriors would have spellBooks aswell. This goes against the Open-Closed principle, since if I wanted to add a Warlock, who can also collect Spells, I would have to modify Spell to try and cast it to Warlock aswell.

Could you please suggest a solution or design pattern, so that I can collect my Collectables without violating the Open-Closed princible?

CodePudding user response：

This is a lot of fun thinking about this. The other answers here definitely address your issue already, but I think in the grand scheme of things you need to change your architecture to something like MVC (model view controller) or SAM (state action model). These will give you a better idea of how to make up classes, b/c right now it seems like you are trying to model your world in terms of physical objects, which is NOT what OOP is about. OOP is about the transfer of data.

With MVC, it might look like:

Model:

public class Spell extends MagicCollectable {
    // attributes like damage or healing
}

public class Relic extends PhysicalCollectable {
    // attributes 
}

public class Wizard extends Character {
    //stores spells
    public Set<Collectable> spellBook;
}

public class Warrior extends Character {
    //fields and methods ommited
}

Controller:

public class WizardController {
    private Wizard wizard;
    public void collect(MagicCollectable collectable);
}

public class WarriorController {
    private Warrior warrior;
    public void collect(PhysicalCollectable collectable);
}

So in your game loop you would actually be instantiating WizardController to embody your character. Also notice, like the other answers, I'm creating more specific models.

CodePudding user response：

There is no reason public Set<Collectable> spellBook; shouldn't be public Set<Spell> spellBook;

Best thing to do would be to make an Interface:

interface SpellCaster{
    void addSpell();
    //other methods
}

and make any character that should be able to collect spells implement this interface.

Edit: And then the collect method in Spell should look like:

@Override
public void collect(SpellCaster character) {
    character.addSpell(this);
}

Although you probably should rename the method. You have two collect methods that are doing something completely different.

CodePudding user response：

I find it interesting that you decided to let the collectable decide where it goes. In a real world scenario wouldn't a Character collect the collectable and for example a wizard collect the spell?

public class Wizard extends Character {
    //stores spells
    private Set<Spell> spellBook;

    public void addSpell(Spell spell) {
        spellBook.add(spell);
    }
}