Saturday, July 14, 2007

GiveItemEffect

NNF = No New Failures.

Distractions
 Could Bookworm Adventures (from Pop-Cap) be any more addictive? The last two times I've sat down to code, I've found myself fighting ancient greek villains instead. Fortunately, I've got good notes from my last session, so I can start right in.
 
Create Stage Pass Item
 I feel like this should be easy. Let's see. Creating test, done. Creating setup function to pass test. Done. Now snapshotting debug text. Done. Test passes.
 I note along the way that my other objects use the Canonical Items collection. I'll need to fix that later.

Implement Give Item effect
 Ok, let's create a "compare to snapshot" test for the Give_Stage_Pass effect. Pretty easy copy/paste three lines of code. Test fails as expected Good. Note that this test only checks to make sure the object talks the talk. I need another test to prove that it walks the walk.

test GiveStagePass Effect
 Ok, so I've written testGiveHomerStagePass and it's failing. Now to implement it so that it passes. Ah, hmm... Here one part of my design collides with another. One the one hand, the effect only knows about the itemID (not the item object). But, the Character wants to get an actual item object (not just the ID). I'll have to consider how to resolve this.

More next time.

Sunday, July 08, 2007

Removing Canonical Abilities III

On our continuing quest to get rid of all Canonical objects...
 I removed it from CCharacter. Currently all tests pass. Looks like I can get rid of the class definition now. Let me try a rename first.
 Nope, testItemAbilityUnicity still needs it. I'll take a look. Interestingly, CItems has an instance of it, but doesn't seem to use it. Deleting from there. NNF (No New Failures). Now doing some actual deletions (instead of renames). Done.
 Looking back on the list of things I thought I'd have to do in order to get rid of this class, it turned out that most of them were unnecessary.
 Next step: do the same for CanonicalStatistics? I'm torn between additional refactorings and adding functionality. Variety is the spice of life, so let's go back to adding functionality (until I find another necessary refactoring.

testCreateReactionToUkelelePlaying
 Ok, back to my last failing test. First, I'll need to create the ability "Play", and associate it with the Ukelele. I'm going to say that it takes an Int of 8 to play. Hmmm, this should be tied to the Item, not the ability. For example, it may take an Int of 8 to play the Ukelele, but an Charisma of 9 to play the Kazoo. This is something I'll struggle with later.
 Ok, I've created the Play ability and included a reference to it in my test. Now I need to add SetPromptAbilityID to the ScriptLine object. Done. I'm able to set the ability to be responded to, but now I need to create and test prompting for an ability. I'm also updating the DebugPrint as I go along, so I may need to re-snapshot. Yup, testAddResponseEffectToScriptLine needs to be updated. Updating ... Fixed.

Item, no ability
 Looking at it now, I'll need to distinguish between my old prompt (just for item) and my new prompt (for a specific Item's Ability). While I am initially tempted to copy FindResponseByItem and replace it with multiple variations (e.g. FindResponseByItemAndAbility,  FindResponseByAbility), I'd like to write FindResponseByItemAndOrAbility (perhaps that's too wordy?). There may be a better way to express it, without getting too vague (FindResponseByParameters). I'm going to go with FindResponseByItemOrAbility, with the excuse that it's an inclusive OR.
 This is actually a pretty straightforward change. Rename everything, add a parameter. I'll do the rename first. Test - NNF. Now adding a parameter (which will be NO_ABILITY in all existing cases). So far, so good. Ah, now that I'm trying to match on (what should be the default value), I'm no longer able to find reponses to just the item (Ukelele in this case).
 This error actually uncovered an older bug and a missing test case. I didn't have any test cases for asking about an item that the NPC did not know about. While I need to fix this to make my other tests pass, I'm also going to take advantage of (and I'll quote) Michael Fowler's Refactoring sound bite on page 97:

When you get a bug report, start by writing a unit test that exposes the bug.

testStageManagerDoesNotRespondToBook
 Writing...Done. And it passes, as I already fixed the bug. Well, I guess I don't follow directions in order all the time (: Now to continue my diagnosis.

testAskStageManagerAboutUkelele
 It's odd that this is failing, as I know that the stage manager knows about the ukelele. I'll throw in a debugprint just to make sure I didn't mess up the object somewhere along the way. Shocking! My NPC class doesn't expose a DebugPrint. Time to fix that.
 Ok. I've implemented DebugPrint, and confirmed that the StageManager is as I expect him to be. Must be a problem with finding the object. ah! Found it. I was comparing an ItemID to an AbilityID, which may match in some cases, but only by accident. Another insidious bug that would have taken a lot more time to find without these tests. Fixed, which fixes those two broken tests.
 Ok, now to make the Stage Manager respond to the Ukelele playing (doesn't it seem like we've been trying to do that for weeks now?). Actually, no, sorry, that was a lie. I've got two more refactorings to do first.

* Get rid of Initilize2 from ScriptLine
 I don't even know why this is still here. Removed..NNF. Good.

Add<object>(<object>)
 This is similar to the change I made around canonical objects. I'm currently passing a list of parameters to the container object (NPC), which makes an instance of the containee  (ScriptLine), and then adds it. The problem, of course, is that if I want to change ScriptLine, I also need to change NPC. So, I'm going to fix this by:

* Adding the object as a parameter
* Overriding the object with the parameters passed in (if the object is nothing).
* Changing the calling functions to use the object parameter.
* Removing the passed in parameters.

Adding the object as a parameter
 All callers will initially pass Nothing as the object. Let me try this first, and then I've found some additional changes (already). Finding all the callers - good, there's only one function that uses it. I've moved the function to my working area (shared.asp). NNF.
 Ok, the additional changes I'll need to make are to UpdateScriptLine (which should also take an object). I'll add that to the end of my todo list.
 Replacing references to the parameters...NNF

* Overriding the object with the parameters passed in (if the object is nothing).
 Done.

* Changing the calling functions to use the object parameter.
 Should this ScriptLine object be part of the global parameters array? No, for now, as it seems pretty specific to this scenario (although having a general ScriptLine object to pass around would be useful).
* Removing the passed in parameters.
 Ok, I made a bunch of changes, now time to test. Ok, so far, so good.

* UpdateScriptLine (which should also take an object)
 Perhaps I shouldn't update at all, but just reject it as a duplicate (if the item/Ability combo match). I like this better. Changing code... Done, but this really calls for another test. For this test I will need a script Line object, so I'm going to create one and add it to my global array. I'll use the one I need elsewhere - SCRIPT_LINE_TAKE_DISPLAYED_UKELELE (hard to get a short description for "If you show my the Ukelele, I will take it away from you").
 You'll be proud of me, I'm starting with a failing test testCreateScriptLineTakeDisplayedUkelele (yes, it will be a debugprint comparison test, so don't be too proud of me). Ok, good. Now to (a) make sure this ScriptLine is created before the Stage Manager, and (b) use it in the creation of the stage manager.
 Unexpected error when I did both. I backed out, and will try again. First, moving the ScriptLine creation to before the Stage Manager creation. No error. Good. Now doing a debugPrint before using it in the stage manager creation. TypeName first, matches expectation, now a full debugPrint. That looks good as well. Hmm... okay, now trying to pass it. Very odd, something is clearing out the object underneath me. Investigating. . . Ah ha! My initial (full parameter list) version of AddScriptLine created its own objScriptLine, and diligently cleared it out at the end of the function. I was doing the same, but to the object that was passed in. Fixed, testing...NNF (now to clean up my debug code). Also cleaning up some local object creation in SetupCreateStageManager that is no longer necessary.

testScriptLineUnicity
 All that to write a test to prove that I can remove the UpdateScriptLine function. Creating new testScriptLineUnicity, adding to test suite, testing... failure - Illegal Assignment (don't see those very often). Ah, my AddScriptLine function wasn't set up to return any values. Easily fixed. (oops, syntax error, also easily fixed) - darn it, another syntax error. Fixed...Phew. Ok, NNF. I'm up to 48 tests (:
 I've finally removed the Update function. It was a good idea at the time, but isn't really maintainable, and isn't consistent with my design choices elsewhere.

Look to the future.
 I've created more scaffolding this session, and I've added an ability to the ReactionToUkelelePlaying function. I still need to add an effect. But I'm not sure what the effect should be - give a stage pass, or finish an objective? While I'm excited about the opportunities to create a new Objective class (Items are yesterday's news), I think I should implement the "Give Item" type of effects first. Short todo list:

* Create Stage Pass Item
* Implement Give Item effect
* Create GiveStagePass Effect

 Also, soon I'll need to hook these objects up to a database. I'm a little worried about that step, but I think it will soon become painful to maintain this growing arrParameters list. Plus, my setup functions will become "load from database" functions. Then I'll need to write a bunch of write to database functions. Then I can finally start creating objects via the UI instead of hardcoded setup functions. Of course, the downside is that any object design changes may require database changes.
 I should write up some common database refactorings (migrate data to a new schema, point to the new database table, drop the old table).
 See you next time.