I just didn’t think about. Thanks, Roy, for pointing it out.Since the String Calculator kata was posed by Roy Osherove, I asked him to take a look at my solution and give me feedback. He was kind enough to write a blog post with some comments. Since he took the time to write them, I thought it fair to weigh in with my responses.
Partial Test: at some point, Corey seems to be writing the tests incrementally as well, in that he writes an empty test that passes, and then fills in the test itself to see it fail (which is usually where I start). I wasn’t sure why he’d do that, but the reasoning could be that he wants to make sure the environment works for the test. since he does it all in Ruby, which is a more forgiving environment for typing errors, this might be it.
Always passing test: very early on, in the second test actually, Corey creates a test that asserts that sending “0” returns “0” – which passes and never fails. That is because by default empty strings as input return zero. I do it a bit differently – i try to start with a failing test that drives more functionality, and if i can find a way to make a test for “0” fail as well to being, i will do it (so that i can ‘test the test’ in TDD style).
This is a great point. The strict TDD cycle is red-green-refactor. I like his point of using a “1″ to watch it fail, then add the #to_i call to make that pass.
Why did I put a “0″ that was automatically green? Perhaps, in my mind, that was a test, as opposed to a design-pressure. I generally say that I TDD my way to the design through writing examples, then write tests after I have the code to catch edge cases, etc. During the process of writing the code, this isn’t always immediately obvious. However, Roy’s point raises the question of “was the ‘0′ case even a necessary test.” My answer? Probably not.
One of the things that I like to bring up when facilitating Code Retreats is Braithwaite’s “TDD as if you meant it” exercise. This involves only writing the simplest example and the simplest code to force out the abstractions and design. This involves thinking about every example and its place in the design process. I clearly did not do this when I wrote the “0″ case.
Magic inputs: Corey sends in various seemingly random, out of the air numbers as inputs to the tests starting from the third test onwards. For example, to a reader who sees a test that says “sending in 5 returns 5” that reader might ask themselves “why 5? why is 5 so special? should I also use 5 in my tests?”.
A different way: To avoid such confusion i like to stick to the lowest common version of an input that still proves the application wrong. if sending in “1” produces the same result as sending in “5” then my all means i will send in “1”. If the input is 2 digits, I’d send in “10”, “100” and so on. If the input still does not make sense I would put it in as a variable named “SINGLE DIGIT NUMBER” to explain to the reader what is so special or not so special about this value.
This is a good point from Roy. Watching through the performance, I can definitely see how this is strange. I’m not a fan of using “1″, “10″, “100″ as examples, since they feel too similar in structure. However, passing “5″ isn’t much better without an explanation. Perhaps a better case would be to change my example description to “handles single digit number,” rather than “returns 5 for 5.” I like the idea of changing the variable name, as well, for further explanation.
The simplest Test:Corey wrote the tests for inputs with \newline characters without considering the most basic test for this kind: a simple string containing only a new line character (equivalent to an empty string). I agree that not everyone would pick up on this as an idea, since you might feel this is an invalid input into the system. nevertheless, when I teach TDD this is the way i teach the kata. It feels like a more natural incremental evolution of the code and tests, and a good way to send the message that anything can be divided into small increments if you want to.
Wow! Great catch. I was so far along in the implementation, definitely already in the flow, that I didn’t think to drop back to such a simple case. I’d like to claim that I felt ‘this is an invalid input into the system,’ but I didn’t. I just didn’t think about.
Roy’s point about this being a good way to send the message of dividing into ever-smaller increments is great. When I teach people, I often ask them the same question.
Refactor to un-readability: At some point Corey refactors the code that handles delimiter characters into a single line of Ruby code. while it is a form of refactoring, i found that the code after refactoring was much less readable than it was before.
This is a wonderful point. I laughed when I read it, because I had brought up the exact same point not too long ago while reviewing someone’s clojure code. Their response was ‘if you knew clojure better, it would not be cryptic.’ I sometimes accept this as an answer, but only rarely. Within reason, I feel, truly well-written code can be understood with minimal fuss by an experienced developer, no matter what the language.
(update: Tracy Harms has called me on the above statement that some languages are not necessary readable by an experience developer with no prior knowledge. So, I gracefully concede that APL and J are decidedly NOT readable. Of course, after seeing bits of both, I will question whether those languages are readable by ANY developer. just kidding. I still hold that most mainstream, not codified math-based, languages should be readable. )
However, the ‘if you knew x better, you would not think it unreadable’ argument can be valid in some cases. Is this one of those cases? I thought I would take a look at the code and see what I think.
self[0,2] == '//' ? delimiter = self[2,1] : ','
After being away from this code, my first impression is that the indexing is a bit domain-specific, but not domain-specifying. In order to understand this line, you need to know that the first two characters equaling ‘//’ imply that the next character is the custom delimiter. If I were helping someone improve this code, I would ask whether it would be better with something like
has_custom_delimiter? ? customer_delimiter : DEFAULT_DELIMITER
In fact, after reviewing this code, I realize that the clause (delimiter = ) is completely unnecessary and left over from a previous step. Naughty me! This goes to show that mercilessly refactoring your code can sometimes highlight things that you might miss by visually looking at it.
So, to answer the question posed above: no, this isn’t a case of ’if you knew x better, you would not think it unreadable.’ This was simply a case of not cleaning up enough. Thanks, Roy, for pointing it out.
On A Side Note
I want to address the idea of feedback. As I wrote in ‘What are Katacasts,’ a major reason for performing katas is to elicit feedback from your peers. None of us are perfect, and, in fact, most of us continue to seek insight into coding through sharing our ideas with others. By receiving specific and pointed feedback, such as what Roy gave me, it helps us improve both by learning new ideas and being reminded of fundamentals that we might have forgotten. I want to thank Roy for taking the time to provide this.
In fact, I would challenge all of us, myself included, to take the time to provide feedback to each other in the form that Roy did for me. The Software Craftsmanship Manifesto states
Not only individuals and interactions, but a community of professionals.
For me, being in a community of professionals means that we strive to help each other improve. Feedback on code kata performances, either here on katacasts, at a user group or someone’s personal blog, is an essential part of this.