Re-factoring SpellAbilityAi

[excessum]

I am thinking of re-factoring canPlayAI() at least into a template pattern as there is a lot of duplicated code across the various implementations of SpellAbilityAi. For starters, it will have
1. check specific card AI
2. check AiLogic param
3. check extra cost (checkSacrifice/DiscardCost etc.)
4. check phase restriction logic (eg. instant draw-effects in opponent turn)
5. rest of logic

Having separate methods for each of the above steps to be overridden as required in the specific implementations should reduce the duplicated code and potential bugs (eg. r31202 where the AI will always discard to activate Stern Constable).

Would it be better to modify the abstract SpellAbilityAi.canPlayAI() directly or just extend it into a new class (eg. SpellAbilityAi2)?

[Hanmac]

I say yeah for refactoring!
That maybe would do other cards easier to code for the AI.

Hm we might do the AILogic a bit more generic like that you can do it that it should only activate something when the Ai does control a creature. (Like the emblem from Ob Nixilis of the Black Oath only makes Sense when the AI also has something to sacrifice)

Like for example with Dazzling Reflection, I can image 3+ use cases when the AI should activate this spell. (That spell is so interesting I want to code it for the AI)

For how you should do it, I dont know either you do a big change or you do SpellAbilityAi2 and changes the AI step by step. Both things would work.

[excessum]

If I remember my Java correctly, changing SpellAbilityAi.canPlayAI() directly will not affect the existing code since it was originally coded as an abstract method (ie. they override it completely). This means that I or anyone else can simply update any of the implementations (eg. DrawAi or EffectAi) by replacing the specific canPlayAI() with the new sub-methods if necessary.

[Hanmac]

Oh ok I didn't know it. I am not that good in Java yet (but have exp in other languages)

I thought an abstract class does need to be pure abstract.
Good that it works.

[friarsol]

Right, changing it this way doesn't actually change anything in practice here since every single EffectAI class has already overridden the base function. We can "unspin" the simpler AI functions in a fairly straightforward way to the 5 new default functions:

checkSpecificCardLogic()
checkAiLogic()
willPayCosts()
checkPhaseRestrictions()
checkApiLogic()


I think I'd prefer using AILogic in all appropriate scripts instead of having a "SpecificCardLogic" function/coding the way some cards do now, that way if the effect is stolen by a different card, it still has the appropriate logic attached to it.