Deduplicating Some Common Check Negated And Shuffling Logic

[foohyfooh]

No description provided.

[mercury233]

Thanks for your contribution, but I'm afraid I won't be able to review this anytime soon.

Large-scale refactoring PRs have always been somewhat controversial. At the moment, I tend to be on the side of those who believe that contributors should discuss significant changes with the maintainers before starting the implementation. Maintainers often have different plans and implementation directions for the future of the project, and these can differ substantially from what the PR author has in mind.

With the rise of AI-assisted programming, the potential cost of wasting the PR author's time and effort has been greatly reduced. On the other hand, the review burden on maintainers has often increased. So my position on this hasn't really changed.

That being said, for brand-new decks, I'm quite open to contributions as long as they aren't obviously incomplete. However, for decks that already have active maintainers, I usually prefer to keep changes minimal.

I do have plans for some refactoring myself in the near future, and the changes in this PR are broadly in line with those plans. That said, I haven't decided yet whether I want to make these kinds of changes to decks with active maintainers.

[mercury233]

@DaruKani @Wind2009-Louse What do you think about this kind of refactoring?

[foohyfooh]

Yeah sorry that this is too big and modifies so much. I don't want to cause a problem with the way things are done for this repo so if it is a problem then you can close this.

Large-scale refactoring PRs have always been somewhat controversial. At the moment, I tend to be on the side of those who believe that contributors should discuss significant changes with the maintainers before starting the implementation. Maintainers often have different plans and implementation directions for the future of the project, and these can differ substantially from what the PR author has in mind.

What should I do for recommending changes like this where I would modify several both to refactor things? Should I open an issue and discuss there?

---

As a smaller change (on different PRs), would it be fine to modify things like

• deduplicate the shuffling logic and put it in the utils?
• put the CheckRemainInDeck method in the Executor as a common thing Bot should be overriding?

[DaruKani]

@DaruKani @Wind2009-Louse What do you think about this kind of refactoring?

I like the general idea of moving commonly duplicated functions into DefaultExecutor / Util. It should make new bots easier to write and reduce duplicated code.

However, I don't think this PR should be merged as-is.

Some of these changes are not purely refactoring. There are small behavior differences between the existing deck-specific implementations and the new shared implementations, so changing many actively maintained bots at once could introduce unexpected behavior.

For actively maintained decks, I would prefer either:

1. Add the shared functions first without modifying existing bots, then let each maintainer migrate their bot after reviewing/testing it; or
2. Split the changes into smaller PRs and discuss them with the maintainers before modifying their bots.

So I support the direction of this refactoring, but I think changes to existing actively maintained bots should be kept minimal unless their maintainers agree with them.

[Wind2009-Louse]

What do you think about this kind of refactoring?

When adding AI_Apophis, I introduced resolvedEffectIdList and enemyResolvedEffectIdList precisely to extract the logic that each bot individually implements for detecting whether the opponent has activated a specific card, and make it a shared utility method. I just haven't opened a new pull request yet to modify the existing bots.

I think this PR is too large in scope. If possible, it would be better to split it into smaller pull requests, for example:

1. Create the basic common classes first.

2. Submit separate pull requests for each executor to adopt the new logic.

As a side note, with resolvedEffectIdList and enemyResolvedEffectIdList in place, there is no longer any need for bot-specific variables like enemyActivateMaxxC that track specific cards.