Reflecting Pool: Is this keyword worthwhile?

[jim]

So I've been playing in my local copy with an implementation of Reflecting Pool. It occurred to me that nearly the same code ought to be useful to implement Fellwar Stone and Exotic Orchard, and I am presently planning to make this my first attempt to add a keyword. It would probably look something like

abAddManaLandProduces:$You (for Reflecting Pool)
abAddManaLandProduces:$Opp (for Exotic Orchard)

Plus there has to be some differentiator to cover the fact that Reflecting pool can add colorless and Exotic Orchard can't. According to Gatherer, there are 3 green creatures (Harvester Druid, Quirion Explorer, and Sylvok Explorer) that would also be easy to implement with this keyword. (Only Reflecting Pool is on the request list though).

It seems to me like it would be worthwhile to add it this way, even though it's clearly a low-use-case keyword (unlike the ones Rob and Chris are adding that enable dozens of new cards). If anyone has another opinion, or feedback on the overly long keyword itself , let me know.

[Chris H.]

It seems to me like it would be worthwhile to add it this way, even though it's clearly a low-use-case keyword (unlike the ones Rob and Chris are adding that enable dozens of new cards). If anyone has another opinion, or feedback on the overly long keyword itself, let me know.

`
I think that it sounds like a good idea. As you work on the code you may decide to shorten the name. It should be fairly easy to find and replace the keyword name. Rob is good at coming up with names for keywords.

[zerker2000]

Considering I made "{1], sac CARDNAME: draw 1" merely for Spellbombs, I'd say go for it. CardFactory already has excessively copy-pasted code; a lot of good could come out of e.g. rerouting the whole targeting system through some kind of "CardFactoryUtil.getTargetType(Card, description)", and/or making something similar for simple setTargetAI logic("scariest human creature", "best artifact to animate", etc.).

[Rob Cashwalker]

Yeah, I give it my seal of approval... I'm not coming up with anything solid on names.... It's good enough. Consider the fact that we use "When this card would be put into a graveyard, shuffle it into your library instead" as a limited application keyword.

Edit - on the other hand, I just thought of a cool name - "abAddReflectedMana:1:You|Opp"

[jim]

Edit - on the other hand, I just thought of a cool name - "abAddReflectedMana:1:You|Opp"

That is a cool name, and I think I'll use it. I am terrible at coming up with names and appreciate the help.

[jim]

Considering I made "{1], sac CARDNAME: draw 1" merely for Spellbombs, I'd say go for it. CardFactory already has excessively copy-pasted code; a lot of good could come out of e.g. rerouting the whole targeting system through some kind of "CardFactoryUtil.getTargetType(Card, description)", and/or making something similar for simple setTargetAI logic("scariest human creature", "best artifact to animate", etc.).

Yes, part of my motivation for wanting to do this as a keyword is to avoid even the small amount of copy-paste and hardcoding that would result. I was hoping that you folks would approve -- thanks for your responses.

The targeting system reroute sounds like a good idea, though it would probably be quite a project. I'd rather not sign up for that task just yet...

[zerker2000]

But that's the thing: for the first part at least, after minimal editing to CardFactoryUtil(just a parser of some sort) and SpellAbility.getText, all spells and abilities in card factory can gradually be converted to use it, reducing line count, without any major problems in leaving the rest still using hardcoded ones(at least not until someone comes along and makes an incorrect assumption that All targeting uses it, and/or forgets AI, but in that case, major improvement must already have been made).

[Chris H.]

So I've been playing in my local copy with an implementation of Reflecting Pool. It occurred to me that nearly the same code ought to be useful to implement Fellwar Stone and Exotic Orchard

`

According to Gatherer, there are 3 green creatures (Harvester Druid, Quirion Explorer, and Sylvok Explorer) that would also be easy to implement with this keyword.

`
It looks like the keyword code would need to appear in three of the CardFactories files:

CardFactories_Creatures.java
CardFactories_Lands.java
CardFactories.java

Would it make sense to have only the parsing code in these three files and to call the SpellAbility method which would be placed in CardFactoryUtil.java? This would be similar to what we have done with eqPump and enPump.

I seem to remember someone in the past suggested that we might want to move the SpellAbility methods for the keywords to a separate class file, CardFactoryUtil_SpellAbilities.java?

[zerker2000]

CardFactory_Keywords: All the card factories' code is already mostly SpellAbilities.

[jim]

It looks like the keyword code would need to appear in three of the CardFactories files:

CardFactories_Creatures.java
CardFactories_Lands.java
CardFactories.java

Would it make sense to have only the parsing code in these three files and to call the SpellAbility method which would be placed in CardFactoryUtil.java? This would be similar to what we have done with eqPump and enPump.

That's in line with what I was thinking: the method to manufacture the spell ability would appear in CardFactoryUtil.java, which would be called from CardFactory_Lands and CardFactory_Creatures. I hadn't realized the parsing code would need to appear in all three files, but I guess there probably isn't a way to avoid this -- we dispatch to the sub-factories before we do any parsing.