mage
mage copied to clipboard
magefree
[WIP] Fix copy constructors
Do Condition objects need a copy constructor?
Many (most/all?) of the Condition classes in xmage are enums without any fields and without any constructor at all. I believe they don't need one because Condition is a functional interface.
Do Condition objects need a copy constructor?
Many (most/all?) of the Condition classes in xmage are enums without any fields and without any constructor at all. I believe they don't need one because Condition is a functional interface.
Most yes. And I switched over as many more as I could. But not ALL of them are enums. E.g. ManaCondition.
Most yes. And I switched over as many more as I could. But not ALL of them are enums. E.g. ManaCondition.
I'm definitely not a Java expert, but as I understand it a functional interface by definition only has one method, so it can't have a copy() method. We don't try to copy Condition objects in classes that use them (e.g. we have this.condition = effect.condition, not this.condition = effect.condition.copy() in effect copy constructors).
Because we don't copy Condition objects, classes that implement Condition should probably be "immutable" (as much as objects can be in Java) and to make this clear the ones that do have fields (e.g. ManaWasSpentCondition) should probably make those fields final.
Most yes. And I switched over as many more as I could. But not ALL of them are enums. E.g. ManaCondition.
I'm definitely not a Java expert, but as I understand it a functional interface by definition only has one method, so it can't have a copy() method. We don't try to copy Condition objects in classes that use them (e.g. we have
this.condition = effect.condition, notthis.condition = effect.condition.copy()in effect copy constructors).Because we don't copy
Conditionobjects, classes that implement Condition should probably be "immutable" (as much as objects can be in Java) and to make this clear the ones that do have fields (e.g.ManaWasSpentCondition) should probably make those fieldsfinal.
A functional interface can have only one abstract method (in Condition's case that's boolean apply(Game game, Ability source);) But it can have as many methods as we want. The condition interface itself has a second method (getManaText) with a default implementation.
Also, I don't think that the this.condition = effect.condition is correct. Just because variables are marked as final, does not mean that we can't change them. Take this as an example:
https://github.com/magefree/mage/blob/5adab75324b8bbb9cd2422a77751df30ebcbbe85/Mage/src/main/java/mage/abilities/condition/CompoundCondition.java#L16-L20
conditions is marked as final, but that doesn't mean that I couldn't change what's inside it. It just means that I can't use a line like:
conditions = new ArrayList<>();. Without the .copy() usage we're relying of someone actually treating the variables are write once.