Wednesday, August 04, 2010 4:40:55 AM UTC 
Updated 2010-08-04: I’ve fixed some of my word awkwardness, and added several TODOs at the bottom of the list. For your enjoyment!
The problem with coding dojos is that no one else seems to want to run them. I’ve long desired to participate in a coding dojo where we work through the object calisthenics rules. We’ve attempted it as a group once before, but the results were poor. So poor, in fact, that when reminded of it recently, one participant gave such a look of horror and shouted “oh no!” It was as if he’d seen a chupacabra and was looking to escape out the window. But it wasn’t a chupacabra—he’s just been through our group object calisthenics dojo and suffered a flashback. No worries Michael, you’ll remain anonymous.
Despite distasteful memories and general horribleness surrounding everything I knew about object calisthenics, I plodded onward. Slowly. And, some year or so later, here I am, blogging about it, and here you are, skimming my blog post, reading every fifth sentence or so. I don’t blame you.
I’m working through object calisthenics because I want to try out the rules, and because I’m preparing for the upcoming public Houston TechFest dojo. I plan to use examples from this codebase to explain each of the rules, so it’s important to get it right. You don’t have to agonize over every tiny detail as much as me. Agonizing over code is not a rule of object calisthenics; for this you can be thankful.
The problem with coding dojos is that no one else seems to want to run them. So I’ll do the best I can come October 9th. I hope I’m prepared.
A note about object calisthenics
I wrote this project following the rules (and over-arching goals) outlined in the original object calisthenics essay [Warning: RTF; will blow your mind]. One major problem with choosing KataPotter to solve, is that I solved the problem without creating many collaborating objects. The essay says to “spend 20 hours and 1000 lines writing code that conforms 100% to these rules.” KataPotter is too small.
If I get in a blogging frenzy, I’ll blog in more detail about my experiences, and I’ll go into depth into each rule and how I learned something from it. But, definitely not right now.
I’ve uploaded it to GitHub
I’ll cut to the chase: http://github.com/pseale/KataPotterObjectCalisthenics – this is the 90% finished product. I’ll list the remaining effort below.
Now for the remaining 90%
Obvious things I’ve missed?
Let me know. I don’t know what I don’t know. These are the rumsfeldian unknown unknowns. Help me make them known unknowns, or known knowns, or known known knowns. Whatever they are, let me know.
Allow my custom collections to implement IEnumerable<T>; remove now-extraneous methods.
Originally I decided IEnumerable<T> would be “cheating”, but you know what? It’s a collection. It’s not cheating. I have some dumb code in there because I didn’t allow myself to deal with the collection as a collection.
Is this method signature a violation?
public Money Add(decimal amount);
Notice anything? It (potentially) violates rule #3 Wrap all primitives and strings. The decimal is a primitive, and thus forbidden. I figure though, how else am I going to add two Money objects to each other, if one of them can’t tell the other Money how much it has? That’s just dumb. Too much time already has been wasted thinking about this, and, seriously, how else are you going to add two objects together?
Break up BookCollection.
It’s doing too much. BookCollection should be about adding, removing, and clearing books; it should be a first-class collection and nothing more. All non-collection behavior should be broken into another class. Perhaps several classes, especially isolating anything related to those impenetrable LINQ queries. Rule #4 says that we should have first-class collections. Rule #7 says to Keep all entities small (50 lines or less). Break it up.
Update: I should have been clued in by the fact that I have no less than 4 test files for this class, split by behavior. Consider me thoroughly clued.
Write a console app that works.
Right now Program.cs sits alone, forlorn. It needs to a) get a list of books to calculate, b) run the calculator, c) emit the result. It’s not hard, I’ve simply neglected it. Also, for the record, I don’t have to adhere to any rule craziness when writing the console app.
Strategy pattern abuse? Investigate.
Investigate the *BookSetCostCalculator classes and figure out what the author meant by Rule #2, Don’t use the ELSE keyword. Side note: remember, his rule predated the anti-if campaign. I know that I would not allow such abomination to live in real code I’d check in. I don’t like anything about the calculators. If there’s any way you can see to either a) expand the scope of this Strategy so that it’s used more than one place, and thus justify its existence, or b) at least find better names, let me know.
Combine BookSet with the *BookSetCostCalculators somehow? For your sake, I won’t even attempt to explain my early thoughts.
Null object abuse? Investigate ZeroMoney.
Again we’re hanging with our good buddy Rule #2, Don’t use the ELSE keyword. This time, the essay encourages us to try out using the null object pattern. I think I’m abusing the pattern with my ZeroMoney. I don’t think that’s what null objects are for. Again, the simplicity of the problem has snagged us, and I’ve tried to shoehorn in a null object where I could have done without.
A second issue I have with the null object pattern is: I don’t ever return null anyway, at least not inside code that I control (both the caller and the called). As they say, what’s up with that?
Namespaces
Rule #7 is Keep all entities small. That means ten (10) classes per namespace. My KataPotter solution is small enough that it almost fits in a single namespace, but I should adhere to the spirit of the rule and add some folders/namespaces. Maybe something will emerge.
Update: I still hate that .NET makes it ugly/discouraging for me to name both a namespace, and a class in that namespace the same thing. Take KataPotter.Core.Book (the namespace) and its class Book. Every time I want to refer to the class Book, I have to either put Book.Book or (what I consider worse) use namespace aliases.
What’s the deal with some of those tests?
I don’t know why I was so nervous at the time about .Clone() not working, but I was. So sue me. I think it had something to do with taking baby steps and trying to make .Clone() work while pretending IEnumerable<T> was forbidden. But still, delete some of those tests. And call out the “acceptance tests” for what they are.
Second note: move the dumb one-line SetUp() code into each test. DRY or no, the SetUp() code is hurting readability.
Third note: remove tests that test non-production code. E.g. money.Add() tests cover null values…why?
Fix “the .ToString()” cheating problem.
This will take a little explanation. The problem with Rule #9 of object calisthenics (no getters, setters, or properties) is that eventually something outside your code will want to interact with something adhering to the rules of object calisthenics, without breaking rule #3! Okay.
Okay, let’s do this by example. Let’s say you’re logging in somewhere. There’s a method called public LoginResult Login(Username username, Password password). Now, how do you know if the login was successful? A bool property? No! A method called GetLoginSuccess()? No! You can’t even be clever and put a method on LoginResult called WasSuccessful()—because what would you return? A bool? That would almost make sense, except Rule #3 is “Wrap all primitives and strings.” If you try to do something clever like WasSuccessful(), you’ll have to return another custom object that wraps a bool, and, now you’re back facing the same problem with which you started!
It’s a conundrum.
I got tired of thinking about it, and I figured, “Hey, I’m writing a console app, at least in theory. I might as well implement ToString() and use it as my dead-simple way to smuggle data out of these objects!” And I did.
If you look at the tests, you’ll see that all of them compare strings. And in their defense, they work, and others have resorted to ToString() to test.
If you bend your thinking a little, and pretend ToString() is called ProjectOntoViewObject() that just so happens to return a string every time, maybe that would soothe your mind a tad? Does it?
It still feels like cheating.
As I’m supposed to adhere to the spirit of Rule #3 (Wrap all primitives and strings) but as I’m also supposed to be able to write code that can be observed (thus saving us from the paradox of trees falling in the forest), I’m permitted to break the rules on the edges of the API I’m building. In my case, in this KataPotter solution, this means Book, Money, BookCollection, and RemoveSetResult all have ToString() methods. These are the classes that either a) are at an API boundary, or b) I needed to unit test badly.
There are known alternatives to the “.ToString()” problem, the most popular one for testing being, implement .Equals(). I didn’t like the idea, partially because we tried that at our group dojo with horrible, horrible results, and partially because you still can’t observe the objects in question, though you can throw them at similar objects in a supercollider at very high speeds and observe what happens. It seems like every test becomes a heavy exercise in mocking.
I’ll stew on this one some more. I need to get rid of the cheating, particularly on internal classes where I can use mocks and expectations to figure out what’s going on. Maybe ToString() is legitimate enough on the boundary objects, and may be permitted there. Will continue to stew and advise.
Updated: What is this property doing in there?!? Property? Rule 9? CHEATER!
I have no excuse. My Book objects have a property getter named “Title”, and other objects make use of the Title directly. Oops!
Updated: What is “bool IsEmpty()” doing in there?!? bool? Rule 3? CHEATER!
Guilty again. BookCollection has declared a “public bool IsEmpty()”, which is wrapped by an identical method on another class, which is then used as part of a decision-making process. If I’m correct, it looks like I’m going to have to introduce yet another (abuse of the) strategy pattern to eliminate the bool returns.
Updated: what’s all this dead code doing in there?
As I happily refactor away, I’m making major changes to the internal organization. There are casualties. Were I a careful C# citizen, I would use the internal keyword instead of public, and R# (and for those of you without, FxCop as well) would be able to easily determine which internal methods and classes are never used, and would be able to highlight them for me. Too bad I’m too lazy to change everything from public to internal.
Thankfully, R# also includes solution-wide analysis, which lets me know which public methods and classes are unused as well. So, this is just a reminder to myself to make sure that solution-wide analysis is turned on, so I don’t miss anything obvious.