Closed manumafe98 closed 5 months ago
@sanderploegsma I would like your feedback here, I'm not sure why does tests are failing, locally when I run ``gradle check```all seem to be working ok
The reason the tests are failing is because we made the decision in the past that every stub solution for every exercise should compile cleanly with the tests, so that students can use the test suite from the moment they start instead of having to diagnose compilation errors.
These changes break that setup; the tests can't compile if the necessary classes are not implemented yet.
If you feel strongly that this exercise should be refactored, we need to think about another way to make sure the tests can run on the starter solution. One way of doing so could be to use reflection to invoke the methods being tested, like in the lasagna
exercise.
If you feel strongly that this exercise should be refactored, we need to think about another way to make sure the tests can run on the starter solution. One way of doing so could be to use reflection to invoke the methods being tested, like in the
lasagna
exercise.
Well I definitely think this need a reformat, what are your thoughts about the change?
Maybe we could remove the first task, the one that the user creates the class, and only leave the second one that the user extends with the parent class? Or that will fail as well?
Well you would need to change the tests a bit because statements like these still won't compile because a Warrior
cannot be assigned to a variable of type Fighter
:
Also, if you don't predefine the isVulnerable()
and damagePoints()
methods on both classes, the later tests won't be able to compile either.
I tend to agree with your opinion on this exercise though, I think that the starter solution is too complete and would like for the student to have to do a little bit of work to get the inheritance part working.
I ran into the same thing when adding the logs-logs-logs
exercise, where the tests need to check for specific enum cases but I wanted the student to have to add those themselves, so i couldn't reference them directly. In that case it was pretty easy to solve this with reflection. And we chose to reuse the approach from lasagna
in the wizards-and-warriors-2
exercise, again because we wanted the student to define the methods themselves.
I tend to agree with your opinion on this exercise though, I think that the starter solution is too complete and would like for the student to have to do a little bit of work to get the inheritance part working.
I ran into the same thing when adding the
logs-logs-logs
exercise, where the tests need to check for specific enum cases but I wanted the student to have to add those themselves, so i couldn't reference them directly. In that case it was pretty easy to solve this with reflection. And we chose to reuse the approach fromlasagna
in thewizards-and-warriors-2
exercise, again because we wanted the student to define the methods themselves.
Great, thanks for the feedback, then I would give it a try with reflection
and those two exercises as an example to make it work.
Perhaps the exercise could benefit from a bit more restructuring: what if we reorder them so that the student first implements the Warrior
class fully, then followed by the Wizard
class? So something like this:
Fighter
)toString()
method on the Warrior
class)damagePoints()
method on the Warrior
class)Fighter
)toString()
on the Wizard
class)prepareSpell()
method on the Wizard
class)isVulnerable()
method on the Wizard
class)damagePoints()
method on the Wizard
class)Also there is one problem with the current design: Because the Fighter.damagePoints
method is abstract
, students can't really finish the second step without also implementing the damagePoints
method. If they don't, their solution won't compile.
Maybe abstract classes deserve their own concept, especially since the inheritance
introduction does not even mention what they are. If we then make the Fighter
type non-abstract and provide a default implementation of the Fighter.damagePoints
method the above issue will be solved.
I have the following in mind:
class Fighter {
boolean isVulnerable() {
return true;
}
int getDamagePoints(Fighter target) {
return 1;
}
}
Note:
damagePoints
to getDamagePoints
since the former seems to indicate that an action is taken while there is notisVulnerable
return true
by default because that makes a bit more sense in the context of the exercise:
Warrior
is never vulnerableWizard
is sometimes vulnerableFighter
is vulnerable by defaultPerhaps the exercise could benefit from a bit more restructuring: what if we reorder them so that the student first implements the
Warrior
class fully, then followed by theWizard
class? So something like this:
- Create the Warrior type (this includes inheriting from
Fighter
)- Describe a Warrior (this covers overriding the
toString()
method on theWarrior
class)- Calculate the damage dealt by a Warrior (this covers overriding the
damagePoints()
method on theWarrior
class)- Create the Wizard type (this includes inheriting from
Fighter
)- Describe a Wizard (this covers overriding the
toString()
on theWizard
class)- Allow a Wizard to prepare a spell (this covers adding the
prepareSpell()
method on theWizard
class)- Make a Wizard vulnerable unless they prepared a spell (this covers overriding the
isVulnerable()
method on theWizard
class)- Calculate the damage dealt by a Wizard (this covers overriding the
damagePoints()
method on theWizard
class)
I like this I added an extra task, to make the warrior invulnerable
@sanderploegsma I will wait for your review before applying the reflection proxy
just in case
I think I want to have another look at the copy in the instructions.md, but other than that it all looks pretty neat so far. I suggest finishing this PR so that all checks pass again, and then we can do some finishing touches on the copy.
@sanderploegsma I was thinking, should we add TODO's like the lasagna and wizard-and-warriors-2 to the given code to the student, to remain consistent with the other exercises.
@manumafe98 I pushed some extra commits to clean up the exercise a bit more. Let me know what you think of the changes, IMO it should be good to go now.
@manumafe98 I pushed some extra commits to clean up the exercise a bit more. Let me know what you think of the changes, IMO it should be good to go now.
Yep I would say that looks great as well! Now seems more like a interactive concept exercise for a student!
pull request
closes #2718
The goal is to update the concept exercise so the student is not directly handled the code and has to work with it a bit more.
Reviewer Resources:
Track Policies