Reformat concept exercise Wizards and Warriors

[manumafe98]

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

[manumafe98]

@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

[sanderploegsma]

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.

[manumafe98]

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?

[sanderploegsma]

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:

java/exercises/concept/wizards-and-warriors/src/test/java/FighterTest.java

Line 51 in 89308ce

Fighter warrior = new Warrior();

Also, if you don't predefine the isVulnerable() and damagePoints() methods on both classes, the later tests won't be able to compile either.

[sanderploegsma]

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.

[manumafe98]

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.

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.

[sanderploegsma]

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:

1. Create the Warrior type (this includes inheriting from Fighter)
2. Describe a Warrior (this covers overriding the toString() method on the Warrior class)
3. Calculate the damage dealt by a Warrior (this covers overriding the damagePoints() method on the Warrior class)
4. Create the Wizard type (this includes inheriting from Fighter)
5. Describe a Wizard (this covers overriding the toString() on the Wizard class)
6. Allow a Wizard to prepare a spell (this covers adding the prepareSpell() method on the Wizard class)
7. Make a Wizard vulnerable unless they prepared a spell (this covers overriding the isVulnerable() method on the Wizard class)
8. Calculate the damage dealt by a Wizard (this covers overriding the damagePoints() method on the Wizard class)

[sanderploegsma]

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:

• I renamed damagePoints to getDamagePoints since the former seems to indicate that an action is taken while there is not
• I made the isVulnerable return true by default because that makes a bit more sense in the context of the exercise:
  • A Warrior is never vulnerable
  • A Wizard is sometimes vulnerable
  • Any other Fighter is vulnerable by default

[manumafe98]

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:

1. Create the Warrior type (this includes inheriting from Fighter)
2. Describe a Warrior (this covers overriding the toString() method on the Warrior class)
3. Calculate the damage dealt by a Warrior (this covers overriding the damagePoints() method on the Warrior class)
4. Create the Wizard type (this includes inheriting from Fighter)
5. Describe a Wizard (this covers overriding the toString() on the Wizard class)
6. Allow a Wizard to prepare a spell (this covers adding the prepareSpell() method on the Wizard class)
7. Make a Wizard vulnerable unless they prepared a spell (this covers overriding the isVulnerable() method on the Wizard class)
8. Calculate the damage dealt by a Wizard (this covers overriding the damagePoints() method on the Wizard class)

I like this I added an extra task, to make the warrior invulnerable

[manumafe98]

@sanderploegsma I will wait for your review before applying the reflection proxy just in case

[sanderploegsma]

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.

[manumafe98]

@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.

[sanderploegsma]

@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]

@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!