Next day it was time to put the fixing code in. Reviewing the above huge blob, we felt that it was too hard to understand to be sure it really worked, so we decided to put it into an object. Let’s review that object and (I hope) you’ll see how much easier it is to understand.
Well, we’ve got to process all the people in question. We decided to pass a Boolean down to say whether to fix a Person once found. The top level code looks like this:
Person class>>process: aBoolean file := 'duplicatePartsTEST.txt' asFilename writeStream. self people do: [ :eachPerson | self processPerson: eachPersonfix: aBoolean]. file closeOK, we’re going to process each person. Easy enough. What do we do to them?
processPerson: aPerson fix: aBoolean | binSupply | personPrinted := false. binSupply := aPerson binSupply. binSupply corporations do: [ :eachCorp | self processCorp: eachCorp binSupply: binSupply person: aPerson fix: aBoolean]
OK, no big deal. We set a print flag (an instance variable of our object), get the Person’s BinSupply, and for each Corporation, process the Corp. That looks like this:
processCorp: aCorp binSupply: aBinSupply person: aPerson fix: aBoolean self binNames do: [ :eachBinName | | bin | binNamePrinted := false. bin := aBinSupply bin: eachBinName corporation: aCorp. self processBin: bin transactionDictionary: Collection identityDictionary person: aPerson fix: aBoolean]
OK, we get the binNames (we’ll address that later), and for each one, get the Bin for this Corp, and process it. We pass in an empty IdentityDictionary for him to process into.
processBin: aBin transactionDictionary: aDictionary person: aPerson fix: aBoolean aBin partsDo: [ :eachPart | self processPart: eachPart transactionDictionary: aDictionary]. self processDictionary: aDictionary bin: aBin person: aPerson fix: aBooleanHere we go through the Bin’s Parts and process each one into the Dictionary. Finished with the Bin, we then process the resulting Dictionary to produce the report and optionally apply the fix. Here's how we process each part:
processPart: aPart transactionDictionary: aDictionary | dateDict amountDict collection | dateDict := aDictionary at: aPart transactionPart ifAbsentPut: [Collection dictionary]. amountDict := dateDict at: aPart effectiveOn ifAbsentPut: [Collection dictionary]. collection := amountDict at: aPart amount ifAbsentPut: [Collection orderedCollection]. collection add: aPartWe process the part by just putting it into our Dictionary of Dictionaries, the same as before. This code is still tricky, but at least it isn’t in the middle of a huge method doing something else.
We still need to see what the processing of the Dictionary is about:
processDictionary: aDictionary bin: aBin person: aPerson fix: aBoolean aDictionary values do: [ :eachDateDict | eachDateDict values do: [ :eachAmountDict | eachAmountDict values do: [ :eachCollection | eachCollection size > 1 ifTrue: [self printCollection: eachCollection bin: aBin person: aPerson. aBoolean ifTrue: [self fixCollection: eachCollection bin: aBin]]]]]This code is still too difficult to read, and frankly should be refactored further. At the time, we were on top of it and it seemed good enough. As I write this I see that we should have gone further. Probably what we really needed was some kind of classification object, not these nested dictionaries. But to proceed ...
All this is doing is going through each transaction dictionary, and each amount dictionary, and if the collection size is bigger than one, printing (and optionally fixing the Bin). Here’s printing:
printCollection: aCollection bin: aBin person: aPerson aCollection do: [ :eachPart | personPrinted ifFalse: [personPrinted := true. file cr; nextPutAll: aPerson messageHeader; cr]. binNamePrinted ifFalse: [binNamePrinted := true. file nextPutAll: '*** '; nextPutAll: aBin name; cr]. file nextPutAll: eachPart printString; cr]. file nextPutAll: '-----'; cr
The print flags, personPrinted and binNamePrinted, are initialized in the outer methods. We used the Caching Instance Variable pattern.
Now here's the fixing code:
fixCollection: aCollection bin: aBin | stream | stream := aCollection readStream. stream next. [stream atEnd] whileFalse: [ | partToRemove | partToRemove := stream next. file nextPutAll: 'removing '; nextPutAll: partToRemove printString; cr. aBin removePart: partToRemove]. file nextPutAll: '=====';cr
We just make a stream on the collection, skip the first item, and remove all the rest from the Bin, printing as we go. At the time, the stream approach came to mind first, so we went with it. An alternative might be:
fixCollection: aCollection bin: aBin 2 to: aCollection size do: [ :each | partToRemove := aCollection at: each. file nextPutAll: 'removing '; nextPutAll: partToRemove printString; cr. aBin removePart: partToRemove]. file nextPutAll: '=====';crOr perhaps:
fixCollection: aCollection bin: aBin (aCollection copyFrom: 2 to: aCollection size) do: [ :each | file nextPutAll: 'removing '; nextPutAll: each printString; cr. aBin removePart: each]. file nextPutAll: '=====';cr
There's not much difference in these three ways of doing the fix collection. Use your favorite.
Here's the real difference that I'd like you to see. Go back and look at each of the methods above. You should be able to understand each one pretty well, even knowing next to nothing about the Persons, Bins, and such. Unless your personal stack is a lot deeper than mine, you understand them a lot better than the preceding bad example.
In our system, we have found that even ad-hoc fixes are worth doing right. They always have to be done at least twice, once in the test database and once in the real one. All too often, if we just do them in a workspace or a series of inspectors, they still have to be done again. All too often, a similar need arises later.
If even the original authors go back to a workspace implementation a week or so later, there is little chance they'll understand it. A well-factored object implementation is easier to understand, easier to apply again, and more likely to contain reusable code for next time.
So ... do it in an object!
© 1997, 1998, Ronald E Jeffries
ronjeffries@acm.org
http://www.xprogramming.com