-
Notifications
You must be signed in to change notification settings - Fork 799
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
[WHO] Implement The Second Doctor #13435
base: master
Are you sure you want to change the base?
Conversation
Player player = game.getPlayer(playerId); | ||
if (player != null) { | ||
if (player.chooseUse(Outcome.DrawCard, "Draw a card ?", source, game)) { | ||
player.drawCards(1, source, game); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to check for prevention/replacement right? drawCards returns an int, only add restriction effect if > 0
(then could combine these four conditions into a single if statement)
@Override | ||
public boolean apply(Game game, Ability source) { | ||
Player controller = game.getPlayer(source.getControllerId()); | ||
if (controller != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a style thing, but it's easier to read with less nesting, e.g.
if (controller == null) {
return false;
}
// then continue code path here for non null case
} | ||
} | ||
|
||
class TheSecondDoctorCantAttackEffect extends RestrictionEffect { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you base this class off of existing code?
|
||
@Override | ||
public boolean isInactive(Ability source, Game game) { | ||
return game.getTurnPhaseType() == TurnPhase.END && this.isYourNextTurn(game); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure this is correct? what if the affected player takes an extra turn?
} | ||
// defender is a permanent | ||
if (game.getPermanent(defenderId) != null) { | ||
return !game.getPermanent(defenderId).getControllerId().equals(controllerId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to declare a permanent and null check it to ensure you're getting the same object
also can simplify with '.isControlledBy()`
Part of #10653 .
Implement [[The Second Doctor]].
Card was tested and seems to work fine.