Player.chooseTarget must be reworked to be compatible with target settings (random)

[Exhora58]

When the Deck of many things rolls a 1-9 you can choose a card from you graveyard to get back into your hand, it is supposed to be random.

[JayDi85]

It's interesting bug.

• target.chooseTarget do a full checks with random and empty possible targets (e.g. will not ask on nothing to choose)
• player.chooseTarget do a short checks without random (well, it's just a part of target.chooseTarget) -- will ignore random and will ask all the time even with empty possible targets.

Card can be fixed by call replace, but there are many other cards with same calls. I think it must be modifed to move some target.chooseTarget code to player.chooseTarget (random, empty possible targets).

P.S. It must be moved to all players (human, computer), not just human.

[weirddan455]

Is it necessary to have both of these methods that seem to do the same thing? Or would the best fix be to remove one of these and refactor the code to use the other (so we only have one method we use everywhere)?

[JayDi85]

I don't known. Need deep research. But I'm sure refactor can broke some test/computer logic in unit tests.

[JayDi85]

would the best fix be to remove one of these and refactor the code to use the other (so we only have one method we use everywhere)

Yes, it's the best solution (if possible).

[awjackson]

It's interesting bug.

* `target.chooseTarget` do a full checks with random and empty possible targets (e.g. will not ask on nothing to choose)

* `player.chooseTarget` do a short checks without random (well, it's just a part of `target.chooseTarget`) -- will ignore random and will ask all the time even with empty possible targets.

Card can be fixed by call replace, but there are many other cards with same calls. I think it must be modifed to move some target.chooseTarget code to player.chooseTarget (random, empty possible targets).

On the contrary, I think that if cards are directly calling player.chooseTarget rather than going through target.chooseTarget then those cards were simply implemented wrong and should be changed, no matter how many of them there are. We should be looking for ways to remove spaghetti code and duplicated code from the core rules engine, not adding more of it to support poorly-coded cards. Random targetting should not care whether the player is human or AI (there's no player agency, it's random) so it should be handled before the Player class gets involved at all.