I've been looking through old code as of late and well, it's...it's...EMBARASSING! There I said it. Does anyone else do this? I was looking at my Thesaurus project (my Squeak version and soon-to-be Java version) and was appalled at a lot of the code. So, I spent sometime refactoring and getting the code back into shape. My Thesaurus project in Squeak was a quick excursion into writing Morphs and that code was fine. What made me crinch was my parsing code! For one thing, I put all of the code for parsing into one class. It was doing all sorts of stuff with the HTML elements that it had no business doing. The first order of business was to move all of the low-level HTML traversal code into a new class. This made the parser much more readable and took away the noise inside of it. The next order of business was the actual traversal code. I made some huge blunders in my implementation of the depth-first/breadth-first algorithm. I knew the first problem was when I couldn't even understand what I was doing!
Here's the original code:
elementsIn: anElement do: aOneArgBlock depthFirst: aBoolean
| left children |
left := OrderedCollection with: anElement.
[left isEmpty]
whileFalse:
[| next |
next := aBoolean ifTrue: [left removeLast] ifFalse: [left removeFirst].
(aOneArgBlock value: next) == false
ifFalse: [children := OrderedCollection new.
next elementsDo: [:each | aBoolean ifTrue: [children addFirst: each] ifFalse: [children addLast: each]].
left addAll: children]]
YUCK! What was I thinking with two checks for depthFirst or not? Here's the new code (which is a new class by the way!):
pvtDo: doBlock depthFirst: isDepthFirst
| searchQueue dyadic |
dyadic := self pvtCurryForContinue: doBlock.
searchQueue := OrderedCollection withAll: self pvtChildren.
[searchQueue isEmpty] whileFalse:
[| next continue |
next := self class on: searchQueue removeFirst.
continue := [next addChildrenTo: searchQueue depthFirst: isDepthFirst].
dyadic value: next value: continue]
Now, you might notice some weirdness like the #pvtCurryForContinue:. All it does is to call the continue block by default if the block only accepts one argument. Otherwise, it let's the block continue through. Here's it's implementation:
pvtCurryForContinue: doBlock
^doBlock numArgs == 1
ifTrue:
[[:each :continue |
doBlock value: each.
continue value]]
ifFalse: [doBlock]
This is my functional programming training shining through. This code is much cleaner and easier to read than the one before.
This was just one simple example. I used the Thesaurus project to refactor to see how enforcing private members in Smalltalk would feel. So far, I found a lot of breaches of encapsulation that I had not noticed in the heat of development. Even with categorizing methods as private and with comments, I still broke the boundaries. The pvt at the beginning makes it obvious and Squeak's compiler tells me immediately when I have done something wrong.
After the refactoring, I felt my design was much cleaner overall. So far, I'm enjoying using pvt at the beginning of my private methods. It makes me get into the habit of "telling" my objects instead of "asking" which I generally do well. But, it helps when I'm weak in the rush to get something done or simply not as focused as I should be. The public protocol is obvious and minimal. Plus, if I have too many pvt methods, I start looking through my methods to break the object down further.
Posts
