Friday, June 15, 2007

Housecleaning

Next steps?
 Now I need to enhance the Stage manager so that he can react to Abilities, not just items. Then I can show the stage manager that I can play the Ukulele, and he'll reward me with a Stage Pass.
 Let's take a look at CScriptLine. So far, it assumes that there's only one type of response - an item goes in, and a string (and, optionally, an effect) come out. I want to make another type of scriptLine, where an ability (optionally with an item) goes in, and a string (and optional effect) come out.  I'd like to establish some valid combinations of input, mostly items and abilities:
 
Prompt   Example
---------  -----------
Item & Ability:  "playing a ukulele"
Item & No ability:  "showing a Stage Pass"
No Item & Ability:  "Wink"
no Item & No Ability:  <invalid>
 
 Ok, so I can set Item and Ability separately, and not have to worry about invalid options (as long as I set one of them). So instead of a adding parameters to the current Initialize function (in half of these cases, one of the parameters would be null), I should expose setItemID and setAbilityID functions.
 A series of copies, pastes, and tests. Then some find/replace to eliminate the Initialize function (and replace it with the better named functions). The result has less odor:
 
OLD CODE
Call objScriptLine.Initialize(lngItemID, strResponse)
Call objScriptLine.SetEffect(objEffect)
 
NEW CODE
Call objScriptLine.SetPromptItem(lngItemID)
Call objScriptLine.SetResponseText(strResponse)
Call objScriptLine.SetEffect(objEffect)
 
 I think I've got all the references, so I rename the function to Initialize2 and test. testAddResponseEffectToScriptLine failed - now to look in other files (I'm not usually this lazy). Last reference fixed, tests all pass. Sweet.
 
 Now I need a new setPromptAbilityID. I feel a little bit of trepidation here, as I'm still not sure of the relationship between Abilities with Items and Abilities without. But I do feel confident that I could change course later with the testing safety net. I also feel like I should rename SetPromptItem to SetPromptItemID at some point (added to rainy day refactoring list). So, first, test.
 What should the story be here? I want the StageManager to react to the Character's ability to play the Ukulele (as opposed to reacting to the Character's Ukulele, which is already done).
 
testCreateReactionToUkelelePlaying
 This sounds like a test of a setupCreateReactionToUkelelePlaying, doesn't it? Should I put ScriptLines into the parameters array (to join items, characters, effects, and so on)?  I don't see much future re-use of this particular script-line object, it's more leaf-nodey. So I'll create it and test it in the same function. If I need to share it later, I'll extract the creation.
 I've written a failing test, but I've also made some unsettling observations about flaws in my previous design choices. I'll add the failing test to my test cases (so that I don't lose track of where I was going), but I think there might be some house cleaning to do shortly.
 The first housecleaning task is to fix up abilities a bit. They currently include a objSuccessfulResultingEffect. I suppose it makes a little sense to think "I play a Ukelele, and it makes music - that's an effect". But, in light of the newly create NonPlayerCharacter object, the result of playing a Ukelele is determined by who you play it at - some people may get annoyed at you, others might want to be your friend, and if you play it in the vacuum of space, well, it has no effect whatsoever.
 I suppose I can leave it in place for now, and let the NonPlayerCharacter override the default behaviour when necessary. After all, At some point an item such as Healing Potion will need to have an effect without a NonPlayerCharacter around. Some verbs are intransitive.
 The second housecleaning task may be the abolition of the Canonical classes. Despite by (considerable) affinity for the word "Canonical", I think these abilities and statistics would be better kept in the global arrParameters (which didn't exist when I came up with the canonical idea originally). Of course, their final home is in a database, which I am mocking up (with little pain so far).
 Let's take a closer look at one of these canonical classes and what it does:
 
CCanonicalAbilities.AllAbilities collection
 Not sure if this is used. Let's try making it private and see what blows up. No additional tests fail. Cool, leaving as private, I don't have to consider this when deciding the fate of this class.
 
Speakers
 As a side note on the continuing deterioration of my tool, my speakers are not occassionally cutting out. I'm 90% sure that the problem is with one of the connecting wires, and not the speakers themselves (although they are rather old).
 
CCanonicalAbilities.FindAbilityByName
 I think that there are only a few places that I look abilities by name, and I can replace those places with either AbilityID (for the lower levels) or a named constant index into the arrParameters array (for the test functions themselves). I'd much rather use arrParameters(PARAMETER_INDEX_ABILITIY_READ_BOOK) than use .FindAbilityByName("Read Book") and hope that there's a space between the words.
 So, this is replacable (and, in fact, provides most of the motivation for making this change).
 
CCanonicalAbilities.ReturnAbilityObjectByName
 Ditto.
 
CCanonicalAbilities.FindAbilityByIndex
 This is just ugly, I think. If I have an AbilityID, I can just get the Ability Object and use its Name property.
 
CCanonicalAbilities.DebugPrint()
 Nothing to see here.
 
 Note that, earlier, I was considering generalizing these Canonical classes. I think it's worth kicking duplicate classes around a bit before investing too much time in their future. Otherwise that earlier work would have been sort of wasted.
 
How to proceed
 I think the first step is to create some new Abilities outside of the CanonicalAbilities framework, and, one by one, replace references to the current abilities with these new free-range ability objects. I'll use IDs that start with 100, just to avoid any collision (which is what the Canonical structure was originally designed to prevent).
 
Unravelling
 FindAbilityByName is used by AddAbility (which exists both in the canonical class as private and in the Character class). I find the first call I want to replace:
 
Call objCharacter.AddAbility("ReadBook", ...)
 
 Instead, I want:
objCharacter.AddAbility(arrParameters(PARAMETER_INDEX_ABILITY_READ_BOOK), ...)
 
 So, I need to create the parameter, and add the function. I'll create the function first. Hmmm... what to do about the name of the function? AddAbility just seems so right. Ok, I'll try replacing the function in place, by adding and then removing parameters.
 
Adding objAbility as a parameter
 There are really two parts to the AddAbility function: (a) make sure the ability doesn't already exist, and (b) actually adding the ability. While the second part will be made easier (instead of retrieving an ability object, I get one passed to me), the first one requires a teensy bit of work. Actually, I can shortcut the work by simply using the name in the object passed to me.
 Oh, that's an idea. In this case, I can just replace the strName parameter with the whole objAbility object in one go. I just need to replace the text name with a call to get it (that is, CanonicalAbilities.ReturnAbilityObjectByName("ReadBook")). Ironic that the first step in getting rid of this object is to increase the number of references to it.
 This is a pretty straightforward replacement, but it'll make the next steps easier. I suppose I should take a minute to lay out exactly what those steps are. After I do this first step, and make sure everthing passes.
 A moment of panic before I run the tests. Are my changes back-outable? Or have I gone too far? Hmm... first few tests run into some problems, but I get by them. Now I'm getting an odd "Object doesn't support this property or method" error. Ah, somewhere I'm passing a CAbility object where I'm expecting a string. I'll start by making the function more defensive (and removing some debug Response.Writes).
 Ah ha! I replaced a AddAbility of an Item instead of character. Easily remedied (although this change will need to affect Items as well at some point in future). Found it. Undid the replace, and now all tests pass.
 Ok, almost time to quit for tonight. But first I'll make a list for tomorrow.
 
* Add CCharacter.FindAbilityByID and update AddAbility to use this instead of FindAbilityByName. Test.
* Replace objNewAbility with the objAbility passed in. Test.
* Create canonical Ability objects as part of arrParameters().
 
* Replace the rest of the calls to the Canonical Abilities object. Test
* Get rid of CCanonicalAbilities.
 
* Repeat all of that for CCanonicalStatistics.
 
* Replace member arrays with "Scripting.Dictionary"s - (actually LookupTables are better for web use). No need to write my own, really.
 
 Ok, see you then.