Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

There has to be a separate refactor for effects that distribute among targets #9457

Open
Alex-Vasile opened this issue Sep 2, 2022 · 3 comments · May be fixed by #13128
Open

There has to be a separate refactor for effects that distribute among targets #9457

Alex-Vasile opened this issue Sep 2, 2022 · 3 comments · May be fixed by #13128
Assignees
Labels
refactoring Developers topics about code and programming

Comments

@Alex-Vasile
Copy link
Contributor

I think this can be merged as is but there probably has to be a separate refactor for effects that distribute among targets

Originally posted by @theelk801 in #9408 (comment)

@Cguy7777
Copy link
Contributor

Cguy7777 commented Dec 2, 2024

I'm going to start working on this.

Here, Susucre proposed creating a new target class TargetUpToCreaturePermanentAmount, which I'm thinking might be the best approach.

@Cguy7777 Cguy7777 self-assigned this Dec 2, 2024
@xenohedron
Copy link
Contributor

I don't think I like that approach.

The TargetAmount classes are already a bit unwieldy as it is due to the way filters are structured:
image
Adding another parameter by separate classes will not be a good inheritance structure.

It may be better to force devs to define all the relevant constructor parameters, such that the base constructor in TargetAmount has a signature of (DynamicValue amount, int minNumberOfTargets, int maxNumberOfTargets) like standard target classes and all the others chain from that (applying appropriate defaults where params can be safely omitted)

Another point - the amount param should be removed from the effect classes and the text generation can take it from the TargetAmount instead, to reduce duplication. (Makes sense to check instanceof TargetAmount anyway.)

Thoughts?

@Cguy7777
Copy link
Contributor

Cguy7777 commented Dec 3, 2024

That sounds like a better approach. I'll start with that idea and see how it works.

@Cguy7777 Cguy7777 linked a pull request Dec 10, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Developers topics about code and programming
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants