[STX] Implement Ecological Appreciation

[htrajan]

No description provided.

[htrajan]

can use player.searchLibrary(...) instead if desired -- went with this approach to avoid having to make two kinds of targets for EcologicalAppreciationTarget: one for TargetCardInLibrary and one for TargetCardInGraveyard

[JayDi85]

Simple xmage hint:

• If you can use existed classes and methods then use it;
• If you need to create a new one then make research and ensure that you really need it;
• Custom classes (card's specific classes) are tech debt and must be skipped as much as possible.

[htrajan]

understood 👍

[theelk801]

It looks fine to me, it'll need to be refactored as a part of #7685 anyway.

[JayDi85]

Do not extends TargetCard -- there are limited number of default target classes, so extends it instead (if you really needs it, but I can't see needs in your use case -- just use TargetCardInYourGraveyard and TargetCardInLibrary with name and cmc predicates).

It's important cause AI uses only known target type and will raise error on the new one (it's not critical in your use case cause it uses existing cards list but in can be critical in another custom targets).

I recommends to use name and cmc predicates to generate dynamic filter and use it in choose dialogs. If you can't then keep that code (it's ok in your use case).

[htrajan]

I removed the EcologicalAppreciationTarget class and just used TargetCardInLibrary and TargetCardInYourGraveyard. I was able to use the cmc predicate to add a filter. However, I wasn't sure how to do the different names predicate, so I extracted that logic to a method and made both targets anon classes overriding canTarget, which seemed to be a good compromise to avoid dupe code. Also, in the GY case, I needed to also block selecting cards that were chosen from library.

[JayDi85]

Create new filter on apply (filter.copy) and add dynamic cmc and names predicates to it.

Name predicate usage example:

• filter.add(Predicates.not(new NamePredicate("ignore name")));

[JayDi85]

Hmm. But I can see the problem with predicates now -- you still allows to select same names in one step, so canTarget is good approach.

[JayDi85]

You can use target.withChooseHint to inform user in choose dialog about current step or additional info. Example: "step 1 of 2, from library".

[htrajan]

added

[JayDi85]

Simple xmage hint:

• If you can use existed classes and methods then use it;
• If you need to create a new one then make research and ensure that you really need it;
• Custom classes (card's specific classes) are tech debt and must be skipped as much as possible.

[JayDi85]

It's a smell code to call shuffle three times. You can store it the status var and use it at the end. Also don't use searched at all -- always shuffle it. Cause cheating possible: user can open choose dialog and disconnect (see #4263) -- so you will get a false here, but he already saw the cards order.

[htrajan]

fixed

[JayDi85]

Missing filter by Objects::nonNull

[htrajan]

fixed

[JayDi85]

If you need names compare then use CardUtil.haveSameNames all the time. It's important to use it in all code cause face down cards don't have names and can't be equal to another empty name (but java's compare does).

[htrajan]

fixed