Removing Canonical Abilities I
I've got a long todo list from last time. Remember, we're trying to remove CCanonicalAbilities. DATP = Done, all tests pass (except for the one outstanding test that has motivated all this refactoring).
* Add CCharacter.FindAbilityByID. DATP.
* Update AddAbility to use this instead of FindAbilityByName. Interesting - testAbilityUnicity failed unexpectedly. Hmm.. my one incomplete oustanding test is interfering with my debugging, I'll remove it for now. Commenting it out, as a reminder to re-add it later. Ah, I fixed a variable name, but left a dangling reference. Fixed...DATP
* Replace objNewAbility with the objAbility passed in. First pass removes about 10 lines. Oh dear! Two tests fail unexpectedly. Hmmm. I see, it turns out that multiple parameters are now unnecessary, but I can't just ignore them yet. Made a few minor changes, which will spawn an additional task to get rid of these excess parameters. DATP. Let's clean up some unused variables for now. 7 lines gone. DATP - Nice.
* Remove lngStatisticID, lngSuccessThreshold from AddAbility in CCharacter.
This will clean up the function even more, but first I need to make the clients pass more intelligent ability objects. As such, I should make the abilities canonical first. So, I'll get back to this.
* Create canonical Ability objects as part of arrParameters().
Let's see which ones I use... ReadBook, which is based on Intelligence, and has a threshold of 11. Creating setupCreateAbilityReadBoook... actually, I can just use a generic setupCreateAbility function, passing whatever I need. It's pretty straightforward, although I wonder if I'm missing something. I feel a little dirty accessing all the properties of CAbility directly, but it's just a data holder for now. At some point, I'll need to push down more responsibility (currently in CCharacter) to CAbility, but not yet.
SetupAbility Coded...typo...DATP. Now to test it with a debugPrint comparison. I'm going to take a moment to remove the purple text (seemed like a good idea at the time). So, this feels a little like cheating - I take a snapshot of the object's debugPrint results, and then hardcode that in the test for comparison purposes. *Shrug* It works for now.
Ok, so I've created a "ReadBook" Ability. I should be able to replace the one I get from Canonical abilities with the one from arrParameters(PARAMETER_ARRAY_INDEX_ABILITY_READ_BOOK). Doing so now... Actually, I can do one replacement at a time. Let's at least test my first two. Ah, one passed, but the other failed. Let's see why.
Ah, I know why - the AbilityID has changed, and I'm not using the same objects consistently. Some of the old references are looking for AbilityID = 0, not 101. Changing my manually created Ability to match IDs with the canonical version. Ah! That makes my hard-coded test fail, of course. It's like playing whack-a-mole. Easily fixed...DATP.
Ok, so now I can go back to doing the rest of the replacements. Done, but two more tests fail. What could be wrong this time...Ah, that's right, reading the book is supposed to have an effect when successful. I'll add that to my Setup script. In fact, that effect can be added to my growing list of arrParameters.
setupCreateStatisticModifierEffect written, added to setupTests. Test, no new failures (TNNF is probably a better acronym. ). Added new test to confirm, and realized that my "Check that this object's debugPrint result matches the expected value" test doesn't need to be 15 lines long. It can be 7 lines instead. Ok, it passes, now to add it to the ReadBook ability (which introduces additional dependencies in the setup function). One line change, did it work? No, new failure in a outdated debug string comparison. Updating... TNNF. (No new failures).
In fact, all my tests are passing now. Phew, that step was more involved than I anticipated. But I got some additional tests and reusable effects and setup functions, so it was worthwhile. It's probably accurate to say that I misjudged the amount of effort required.
* Remove lngStatisticID, lngSuccessThreshold from AddAbility in CCharacter.
Ok, now this should be a breeze. First, let's prove my theory that this are no longer needed. Yup, I made the variables "do nothing" and all tests pass. Now to clean them up. Oh no, I needed to repeat the previous step for all other canonical abilities (such as Hit Thumb). Well, I don't think Hit Thumb is a very valuable (or re-usable) ability, so I'll just make it inline to this function (presuming no other tests use it).
As a side note from reading Refactoring by Martin Fowler, I think an automated tool to help with these refactorings (in fact, to check all the references and then implement it if the coast is clear) would be awesome. Of course, I don't think there will ever be one for ASP, it's simply not worthwhile (:
Ok, I've successfully simplified the signature of this one method. In the process, I've removed one of the dependencies on the canonical ability collection in the character object.
My todo list is still quite long, but it's well structured. For now, time to take a shower and prepare for an afternoon of gaming with Helga's Heroes.
* Add CCharacter.FindAbilityByID. DATP.
* Update AddAbility to use this instead of FindAbilityByName. Interesting - testAbilityUnicity failed unexpectedly. Hmm.. my one incomplete oustanding test is interfering with my debugging, I'll remove it for now. Commenting it out, as a reminder to re-add it later. Ah, I fixed a variable name, but left a dangling reference. Fixed...DATP
* Replace objNewAbility with the objAbility passed in. First pass removes about 10 lines. Oh dear! Two tests fail unexpectedly. Hmmm. I see, it turns out that multiple parameters are now unnecessary, but I can't just ignore them yet. Made a few minor changes, which will spawn an additional task to get rid of these excess parameters. DATP. Let's clean up some unused variables for now. 7 lines gone. DATP - Nice.
* Remove lngStatisticID, lngSuccessThreshold from AddAbility in CCharacter.
This will clean up the function even more, but first I need to make the clients pass more intelligent ability objects. As such, I should make the abilities canonical first. So, I'll get back to this.
* Create canonical Ability objects as part of arrParameters().
Let's see which ones I use... ReadBook, which is based on Intelligence, and has a threshold of 11. Creating setupCreateAbilityReadBoook... actually, I can just use a generic setupCreateAbility function, passing whatever I need. It's pretty straightforward, although I wonder if I'm missing something. I feel a little dirty accessing all the properties of CAbility directly, but it's just a data holder for now. At some point, I'll need to push down more responsibility (currently in CCharacter) to CAbility, but not yet.
SetupAbility Coded...typo...DATP. Now to test it with a debugPrint comparison. I'm going to take a moment to remove the purple text (seemed like a good idea at the time). So, this feels a little like cheating - I take a snapshot of the object's debugPrint results, and then hardcode that in the test for comparison purposes. *Shrug* It works for now.
Ok, so I've created a "ReadBook" Ability. I should be able to replace the one I get from Canonical abilities with the one from arrParameters(PARAMETER_ARRAY_INDEX_ABILITY_READ_BOOK). Doing so now... Actually, I can do one replacement at a time. Let's at least test my first two. Ah, one passed, but the other failed. Let's see why.
Ah, I know why - the AbilityID has changed, and I'm not using the same objects consistently. Some of the old references are looking for AbilityID = 0, not 101. Changing my manually created Ability to match IDs with the canonical version. Ah! That makes my hard-coded test fail, of course. It's like playing whack-a-mole. Easily fixed...DATP.
Ok, so now I can go back to doing the rest of the replacements. Done, but two more tests fail. What could be wrong this time...Ah, that's right, reading the book is supposed to have an effect when successful. I'll add that to my Setup script. In fact, that effect can be added to my growing list of arrParameters.
setupCreateStatisticModifierEffect written, added to setupTests. Test, no new failures (TNNF is probably a better acronym. ). Added new test to confirm, and realized that my "Check that this object's debugPrint result matches the expected value" test doesn't need to be 15 lines long. It can be 7 lines instead. Ok, it passes, now to add it to the ReadBook ability (which introduces additional dependencies in the setup function). One line change, did it work? No, new failure in a outdated debug string comparison. Updating... TNNF. (No new failures).
In fact, all my tests are passing now. Phew, that step was more involved than I anticipated. But I got some additional tests and reusable effects and setup functions, so it was worthwhile. It's probably accurate to say that I misjudged the amount of effort required.
* Remove lngStatisticID, lngSuccessThreshold from AddAbility in CCharacter.
Ok, now this should be a breeze. First, let's prove my theory that this are no longer needed. Yup, I made the variables "do nothing" and all tests pass. Now to clean them up. Oh no, I needed to repeat the previous step for all other canonical abilities (such as Hit Thumb). Well, I don't think Hit Thumb is a very valuable (or re-usable) ability, so I'll just make it inline to this function (presuming no other tests use it).
As a side note from reading Refactoring by Martin Fowler, I think an automated tool to help with these refactorings (in fact, to check all the references and then implement it if the coast is clear) would be awesome. Of course, I don't think there will ever be one for ASP, it's simply not worthwhile (:
Ok, I've successfully simplified the signature of this one method. In the process, I've removed one of the dependencies on the canonical ability collection in the character object.
My todo list is still quite long, but it's well structured. For now, time to take a shower and prepare for an afternoon of gaming with Helga's Heroes.
posted by FlyingSheep @ 12:45 PM
0 comments
