TDD on the Diamond Problem
Updated with a “chapter” cleaning up the code. I’ve decided to leave everything in the same article. It’s over 6000 words now, so if you want to pack a lunch, that’s fine with me. I found everything I did here to be of value and I hope you’ll read and think about it and find value for yourself as well.
Alistair Cockburn has written an article, in which he pushes for more thinking before programming. He bases his article on observations by Seb Rose, who wrote an article that caught Alistair’s eye. Seb’s presentation is interesting, but Alistair went off in a different direction.
I looked at Alistair’s article and got two things out of it. First, I understood the problem immediately. It seemed interesting. Second, I got that Alistair was suggesting doing rather a lot of analysis before beginning to write tests. He seems to have done that because Seb reported that some of his Kata players went down a rat hole by choosing the wrong thing to test.
Alistair calls upon the gods, Dijkstra and Gries, who both championed doing a lot of thinking before coding. Recall that these gentlemen were writing in an era where the biggest computer that had ever been built was less powerful than my telephone. And Dijkstra, in particular, often seemed to insist on knowing exactly what you were going to do before doing it.
I read both these men’s books back in the day and they were truly great thinkers and developers. I learned a lot from them and followed their thoughts as best I could.
They recommended doing a lot of thinking up front. I still recommend doing a lot of thinking but I’m not at all convinced how much of it needs to be up front. Certainly some does, but perhaps not much. I think it’s more important to be thinking all the time. We’ll see how that turns out, because I’m going to try to solve the problem Seb and Alistair were addressing, and I’m going to jump right into the code. (In fact, I’m writing all this introductory stuff after having done the testing and coding. So I’m not secretly thinking all this time.)
One more thing. I looked at Alistair’s article. But I didn’t read it carefully and at first spin through I didn’t understand his up front thinking and I didn’t even try to read his code. It had something in it about trays, I remember that. Anyway, let’s see what happens. We’ll learn something no matter what.
Getting started with hookup
We’re going to write a program that builds a diamond picture like this, for some set of letters from A to whatever:
--A--
-B-B-
C---C
-B-B-
--A--
Naturally, my first test looks like this:
require 'test/unit'
class TestDiamond < Test::Unit::TestCase
def test_hookup
assert_equal(3, 2+2)
end
end
I always begin with a hookup test, because so often I don’t get the hookup right. This time I misspelled “require” and wrote “text” for “test”. I also had the editor set wrong, so it thought it was running rspec instead of test/unit. Other than that, it all went fine. I’ll change the test to run and carry on. I see that I implemented 3 incorrectly. I should have written 2+1. Making that change, the test runs.
First real test
OK, what’s the problem? We get two input letters, A and some other one. We draw the diamond, spacing so that the diamond is rectangular and the letters fan out and then back in.
Since I am thinking, I note immediately that it goes A B C B A and that the closing lines look much the same as the opening lines. So I decide to ignore the closing lines for a while.
What’s a good first test? Honestly, it’s dictated by tradition. We have to test A. Anything else would be over the top. So let me do that test. Remember that a test is Arrange, Act, Assert, and we usually write them in the opposite order. So I want to assert something about the answer. I’m inclined to think that I’m going to generate an array of strings. That’s looking a long way ahead and reaching pretty far but I’ll give it a go. Here’s the Assert:
def test_A
assert_equal('A', diamond[0])
end
I’ve decided that the result is called diamond, and I think it’ll be an array of strings. Now the Act. I guess I’ll posit a DiamondMaker object that has a method diamond, so here’s Act:
def test_A
diamond = maker.diamond
assert_equal('A', diamond[0])
end
Well, that’s what it’d look like to call the maker. Let’s create it for Arrange:
def test_A
maker = DiamondMaker.new('A', 'A')
diamond = maker.diamond
assert_equal('A', diamond[0])
end
That looks good to go. Let’s run it: I’m a good programmer, so it probably works. Oops, maybe not. I need to create the object, so I got a “red bar”. I’ll toss in a minimal class:
class DiamondMaker
def initialize(start, stop)
end
def diamond
return ['B']
end
end
Test doesn’t run, because we’re supposed to be red first. So I put in ‘B’ as the answer. As soon as I put in ‘A’, it should go green. And sure enough, it does.
What’s the second test?
Well, we’re in great shape. We have a test, a first cut at the class and its protocol, and even a correct answer. Time for another test.
The obvious test is A to B. At least Alistair thinks so, or perhaps it’s Seb Rose who thinks so. And A-B will drive out the actual creation of the array, since I just returned a literal. So let’s give it a go.
def test_A_B
maker = DiamondMaker.new('A', 'B')
diamond = maker.diamond
assert_equal('-A-', diamond[0])
assert_equal('B-B', diamond[1])
end
That looks good to me. I had to think about the pattern a bit, if you don’t mind. I notice that the lines were all one character long in the case of just A, and that they are three characters long in the case of B. I wonder if C will make it five. Hm, yes, it’s going to be C—C, isn’t it? But I digress. We have a test to make work. I’ll have to do some stuff. I think I’ll start by saving the letters in member variables start and stop:
def initialize(start, stop)
@start = start
@stop = stop
end
Then I did a little Ruby study to find out how to convert a character (string) to an integer. I’m rusty on Ruby. So sue me. Anyway it turns out that it’s easy, and I think I’ll do it right now:
def initialize(start, stop)
@start = start
@stop = stop
@start_integer = @start.bytes[0]
@stop_integer = @stop.bytes[0]
end
That should come in handy. Should I test it? Actually, I just printed a sample. It’s obscure, so let’s insert an explanatory test:
def test_counts
maker = DiamondMaker.new('A', 'D')
assert_equal(65, maker.start_integer)
assert_equal(68, maker.stop_integer)
end
Author Note: It turns out that those member variables were pretty speculative and some of them are not ideal or necessary. Well, that happens when you’re having fun. Sometimes you write a line of code that’s not required. Deal with it, that’s my motto.
We make the test run with attr_reader :start_integer, :stop_integer. Of course our test for the array values still fails. Time to think again.
OK, it seems to me that in our diamond method, we’re going to have to create an array, containing the appropriate line for each letter between start and end, inclusively. I figure we’ll iterate the integers and convert them back to characters: there’s probably some way in Ruby to do that.
We’ll want to know some other stuff. Certainly we need to know our target length for the array elements. And it seems to me that a good way to go would be to create the element character by character, selecting either dash or the letter involved, at each stage.
Hm, and thinking about that tells me that just as the lines are symmetrical top to bottom, they are symmetrical side to side. Let me explain that. Each line consists of a set of dashes and a letter and some more dashes, up to the center. Then it repeats that sequence backward. If, as I suspect, the lines are all of odd length, then the center item will not repeat.
To digress: are all the line lengths odd, for any ending character? I claim yes: For any size diamond, the first line always consists of k dashes, an A, and k dashes. The length is always 2k+1, which is always odd. So that’s useful.
Digression
This is surely a fact that your friendly neighborhood Alistair (and all those smart people he mentions in his article) would have asked you to figure out before you started coding. Yeah, well, I didn’t and so far I’m doing just fine.
Back to making the test run
Thinking about the lines, we observe that A is always in the middle position. B appears one to the left of A and one to the right. C appears two to the left of A and two to the right, and so on. That’s interesting, isn’t it?
What if we compute the line from the center outward? (There’s always a center: the lines all have odd length for any diamond.) We’d have a really simple pattern: A— -B– –C- —D. The letter would be emitted one place further along. And in fact it would be in the position we might call “letter - A”. What if we just started with a string of the right number of dashes, and dropped the letter in the right place?
I like this idea. I’m going to change my current test to test this. I could “ignore” the test or comment it out but life is short. I think in Seb’s terms, I’m recycling the test. Whatever you call it, here’s the new test:
def test_A_B_left
maker = DiamondMaker.new('A', 'B')
assert_equal('A-', maker.maker-right(0))
assert_equal('-B', maker.maker-right(1))
end
I have posited a new method, maker_right(k), that returns the right side of the k-th line.
Implementing that turned out to be a little tricky. I wrote maker_right this way:
def maker_right(k)
result = self.base_string(k)
result[k] = (k+65).chr
return result
end
As you’ll recall, base_string is supposed to be some number of dashes, namely the number of letters between A and the final letter, inclusive. So I needed base_string, which I wrote like this:
def base_string(k)
result = ''
self.interval.each { |ignored| result += '-'}
return result
end
Of course, interval is supposed to go from zero to the length of the right-side string. (As I look at that now, I see a simplification. We’ll refactor in a moment.) Anyway I needed interval and I didn’t feel confident, so I inserted this test:
def test_A_B_interval
maker = DiamondMaker.new('A', 'B')
assert_equal((0..1), maker.interval)
end
This drives this method:
def interval
(0..@stop_integer - 65)
end
And all the tests run.
Here’s the code so far
require 'test/unit'
class DiamondMaker
attr_reader :start_integer, :stop_integer
def initialize(start, stop)
@start = start
@stop = stop
@start_integer = @start.bytes[0]
@stop_integer = @stop.bytes[0]
end
def interval
(0..@stop_integer - 65)
end
def base_string(k)
result = ''
self.interval.each { |ignored| result += '-'}
return result
end
def maker_right(k)
result = self.base_string(k)
result[k] = (k+65).chr
return result
end
def diamond
return ['A']
end
end
class TestDiamond < Test::Unit::TestCase
def test_hookup
assert_equal(3, 2+1)
end
def test_counts
maker = DiamondMaker.new('A', 'D')
assert_equal(65, maker.start_integer)
assert_equal(68, maker.stop_integer)
end
def test_A
maker = DiamondMaker.new('A', 'A')
diamond = maker.diamond
assert_equal('A', diamond[0])
end
def test_A_B_interval
maker = DiamondMaker.new('A', 'B')
assert_equal((0..1), maker.interval)
end
def test_A_B_left
maker = DiamondMaker.new('A', 'B')
assert_equal('A-', maker.maker_right(0))
assert_equal('-B', maker.maker_right(1))
end
end
Eine Kleine Refactorung
Our tests are green. We can refactor if we want to. It occurs to me that using the interval is wasteful: we’re just going to compute n dashes for some n and we could do that with integer upto, something like
0.upto(something) { |i| result += '-'}
Surely something is @stop_integer - 65
Let’s try that. And it works. I’ll show you the code later, let’s move on. I think there’s probably some more clever way to produce the dashes, such as defining a bit long string of them and cutting it, but that doesn’t interest me just now.
I’m feeling nearly done!
What interests me is that I think we might be getting nearly done. We have the right side of the output line in hand, if my theories are correct. All we have to do is reflect it around its beginning to get the whole line.
Does a string know how to reverse? If so, we can slice, reverse, append. Turns out it does know. So …
def test_A_C_full
maker = DiamondMaker.new('A', 'C')
b_line = maker.full_line(1)
assert_equal('-B-B-', b_line)
end
And of course:
def full_line(k)
base = maker_right(k)
slice = base[1, base.length]
return slice.reverse + base
end
I also deleted the interval test and method, since they were no longer used.
Wow, this is fascinating. I think I’m done for the top of the diamond. Let’s write a test:
def test_A_D
maker = DiamondMaker.new('A', 'D')
diamond = maker.diamond
assert_equal('---A---', diamond[0])
assert_equal('--B-B--', diamond[1])
assert_equal('-C---C-', diamond[2])
assert_equal('D-----D', diamond[3])
end
This fails, of course, because we still have the trivial definition of diamond. This should be an easy implementation:
def diamond
(0..@stop_integer-65).collect { |i| self.full_line(i)}
end
And the tests all run. I note in passing that this method should really be called diamond_top but instead of doing that, I’ll just start testing the bottom lines and extend diamond to work.
Thinking about that for a couple of moments makes me note that we have another reflection thing going on. So if we were to reflect the array that comes back from that collect just above … wouldn’t we have our answer? I think we would. I’ll just extend the current test downward, then make it work.
def test_A_D
maker = DiamondMaker.new('A', 'D')
diamond = maker.diamond
assert_equal('---A---', diamond[0])
assert_equal('--B-B--', diamond[1])
assert_equal('-C---C-', diamond[2])
assert_equal('D-----D', diamond[3])
assert_equal('-C---C-', diamond[4])
assert_equal('--B-B--', diamond[5])
assert_equal('---A---', diamond[6])
end
Fails, of course, until we do this:
def diamond
base = (0..@stop_integer-65).collect { |i| self.full_line(i)}
slice = base[0,base.length-1]
return base + slice.reverse
end
The tests all run. It occurs to me to worry about the single-letter case, however, since I suspect the code above will duplicate the first line. I think I’ll put a length check into the A test and the one I just wrote.
def test_A
maker = DiamondMaker.new('A', 'A')
diamond = maker.diamond
assert_equal(1, diamond.length)
assert_equal('A', diamond[0])
end
def test_A_D
maker = DiamondMaker.new('A', 'D')
diamond = maker.diamond
assert_equal('---A---', diamond[0])
assert_equal('--B-B--', diamond[1])
assert_equal('-C---C-', diamond[2])
assert_equal('D-----D', diamond[3])
assert_equal('-C---C-', diamond[4])
assert_equal('--B-B--', diamond[5])
assert_equal('---A---', diamond[6])
assert_equal(7, diamond.length)
end
They both run. Magic! But I’m surprised, so I have to figure out why. And, guess what? The array slice for the case of length 1 will be base[0,0] and since slice is [start, length] the array is empty, its reversal is empty, and the answer is correct.
This same magic is surely operating in the line reversal as well as the array reversal. Frankly, I got lucky, but I noticed the concern and checked it out. Seems OK to me.
What can we conclude?
It’s time to refactor again, but this article isn’t about that. It’s about thinking up front versus thinking as we test and code.
There’s no way to compare the two cases. Alistair and I are different people and we’re both glad of it. He can’t go back and build the code without thinking up front, he already thought up front. And I can’t go back and do it over with thinking up front, because I already solved it.
However, let’s look at what happened to me, working my way. You can compare with Alistair’s at your leisure.
I was thinking all the time. I tried hard to write down my every thought as I went, so you’d see what was going on in my head. I left out the dirty parts, sorry.
I discovered the symmetry early. It’s hard to miss if you look at the picture, which is symmetrical about both X and Y. I realized that the line lengths were always odd, which turned out not to be particularly important but gave me a bit of confidence that I was understanding the problem.
I started with the well-known and obvious first test. I made it run, with the usual expected result of laying in the basic object and methods. Then I thought about whether I wanted to do A-B,, and how to do it, and I realized it was too big a bite, so I went for a smaller step. I stayed at the level of very small steps for a while, digging in and then building back out.
The line and array reversing are interesting, I think. They come from something other than mathematical thinking, at least the way I did it. I was aware of the symmetry. Symmetry is about reflection. I thought about reflecting the line. I realized that the reflection was equal to the original, reversed.
Well, I don’t know, surely that’s mathematical in some sense, and I do have a couple of degrees in math so the reflexes are still there, even though I got the degree before zero was invented. You kids think you have it tough. Imagine not knowing what to say when you went to look for a potato and the last one was gone. No way to say “we have zero potatoes”. Things were tough back then. But I digress.
Anyway, I envisioned the operation as a slice reverse append kind of thing and Ruby was happy to oblige. The same thing had to be done again and turned out very similar. Both of those steps were quite trivial to code and clear enough to me as the reader. Frankly I think I did it faster that way than I could have done proving it mathematically.
Comparing the approaches
I did this problem TDD style, with as little up front thinking as I could muster, and as much thinking as I worked as I could bring to bear. That’s how I always do TDD. Kent Beck referred to it as letting the code participate in the design. That’s pretty much what Dijkstra and Gries recommended against. They were much more about first design, and only then code.
Math is a formal language and when you think formally, your thoughts tend to be correct. You’re human, so they’re not always correct. You’re human, so sometimes you go down rat holes and don’t discover really good answers. In math, we think about the “elegance” of your proof or solution, and that’s part of knowing how you’re doing.
Programming? Really just the same. It’s surely a formal language, with the special advantage that if you don’t say it right you don’t get the right answer and the computer will tell you. You’re human, and sometimes you go down the rat hole. You realize this because your code gets ugly and you become confused. We think about clean and elegant code, just as we think about clean and elegant math.
We have another advantage in code. We have tests, so we can refactor. It’s possible to refactor a proof, but it’s not easy. In code, we cut this, paste that, recast this line and voila! we have a better solution and the tests tell us it still works.
Bottom line
I’m all for thinking. In so many cases, including this one, I find no big advantage to doing a big blob of thinking up front. I attribute this to just a few things: I’m very sensitive to the code pushing back. I’m sensitive to taking too big a bite. I’m quick to back up and take a smaller bite. And I’m always thinking, not just coding away.
Maybe I’m a bit quicker to see a rat hole and get out of it: I’ve been doing TDD for a rather long time. But what I do – what I did here – is not some kind of advanced technology indistinguishable from magic. It’s plain, simple TDD, with sensitivity turned up, change size turned down, and thinking engaged all the time.
Thinking is good. Thinking up front is fine, but thinking all the time is more important.
Chapter 2
It’s 6 AM and I need to clean up that code. Shouldn’t take long, but I think we can make it better.
As it is now
require 'test/unit'
class DiamondMaker
attr_reader :start_integer, :stop_integer
def initialize(start, stop)
@start = start
@stop = stop
@start_integer = @start.bytes[0]
@stop_integer = @stop.bytes[0]
end
def base_string(k)
result = ''
0.upto(@stop_integer - 65) { |ignored| result += '-'}
return result
end
def maker_right(k)
result = self.base_string(k)
result[k] = (k+65).chr
return result
end
def full_line(k)
base = maker_right(k)
slice = base[1, base.length]
return slice.reverse + base
end
def diamond
base = (0..@stop_integer-65).collect { |i| self.full_line(i)}
slice = base[0,base.length-1]
return base + slice.reverse
end
end
class TestDiamond < Test::Unit::TestCase
def test_hookup
assert_equal(3, 2+1)
end
def test_counts
maker = DiamondMaker.new('A', 'D')
assert_equal(65, maker.start_integer)
assert_equal(68, maker.stop_integer)
end
def test_A
maker = DiamondMaker.new('A', 'A')
diamond = maker.diamond
assert_equal(1, diamond.length)
assert_equal('A', diamond[0])
end
def test_A_C_full
maker = DiamondMaker.new('A', 'C')
b_line = maker.full_line(1)
assert_equal('-B-B-', b_line)
end
def test_A_B_left
maker = DiamondMaker.new('A', 'B')
assert_equal('A-', maker.maker_right(0))
assert_equal('-B', maker.maker_right(1))
end
def test_A_D
maker = DiamondMaker.new('A', 'D')
diamond = maker.diamond
assert_equal('---A---', diamond[0])
assert_equal('--B-B--', diamond[1])
assert_equal('-C---C-', diamond[2])
assert_equal('D-----D', diamond[3])
assert_equal('-C---C-', diamond[4])
assert_equal('--B-B--', diamond[5])
assert_equal('---A---', diamond[6])
assert_equal(7, diamond.length)
end
end
What do we see here? There are some supporting tests that, strictly speaking, are not needed. But they tell a story. We’ll have to look at them more deeply, because I like to retain the story-telling part of a TDD session in the code.
The member variables are almost all useless. We could blame that on Premature Coding Syndrome, writing lines of code that aren’t required by the tests, or we could chalk them up to incremental development. At the time I wrote them, I thought I needed them. But it was indeed speculation.
Let’s start there. A quick editor-assisted look at the code tells us that @start, @stop, and @start_integer are not used in the code, and only @start_integer is even tested. Rip those right out, including the test for @start_integer. That goes well, only took three runs of the tests to get them all. Also two minutes max.
The only uses of @stop_integer seem to be saying @stop_integer - 65. We can clean those up by defining @stop_integer that way at the top and fixing the test. Here’s where we are now, with just the relevant bits included:
def test_counts
maker = DiamondMaker.new('A', 'D')
assert_equal(3, maker.stop_integer)
end
def initialize(start, stop)
@stop_integer = stop.bytes[0] - 'A'.bytes[0]
end
def base_string(k)
result = ''
0.upto(@stop_integer) { |ignored| result += '-'}
return result
end
def diamond
base = (0..@stop_integer).collect { |i| self.full_line(i)}
slice = base[0,base.length-1]
return base + slice.reverse
end
What else?
Well, there really isn’t much point in the constructor passing in the ‘A’ at all: we always start with ‘A’. So we could redefine the interface to the object to just take the final letter. Let’s do that.
That went swimmingly and the changes are obvious. Do you want to see them, or would you rather wait a bit. Wait? OK, we’ll do another. Or two.
The two tests that test the result length have the test for length in different locations. One is first, the other last. I moved the one that was last to be first, for consistency. Consistency helps the reader. And I moved it first because checking for the right number of items is less detailed, so I thought the tests should run from less detailed to more.
Algorithm improvements
What we’ve done so far has been mostly cosmetic. Arguably the little change to @stop_integer was algorithmic, but it felt more cosmetic at the time. Now that the code is a bit more clean, I want to look at the algorithms used.
To begin with, I don’t like this:
def base_string(k)
result = ''
0.upto(@stop_integer) { |ignored| result += '-'}
return result
end
This gives us the standard output line, correct length, before the letter is plugged into it. But it’s weird, and it’s duplication.
Duplication? Yes! Duplication is most commonly found with lines that look the same. But this is duplication in that it happens again and again and it always gets the same result. Let’s do the trick of slicing from a constant string first. Then we might do that in initialize. I’m a bit concerned about when the string gets copied, however. If we actually reused the same line, we’d get something like A---, AB--, ABC-, and that would be bad. Let’s see what happens. I’ll change the code to this:
def base_string(k)
dashes = '-------------------------------------'
dashes[0,@stop_integer+1]
end
Note that we had to use @stop_integer+1 because the square-bracket construct is start, length, not start, stop. We can improve that by using a range thusly:
def base_string(k)
dashes = '-------------------------------------'
dashes[(0..@stop_integer)]
end
An array subscripted by a range pulls out exactly those characters, which is just like we want. The tests still run, of course. Now I’ll try moving that calculation to the initialize.
def initialize(stop)
@stop_integer = stop.bytes[0] - 'A'.bytes[0]
@base = '-------------------------------------'[(0..@stop_integer)]
end
def maker_right(k)
result = @base
result[k] = (k+65).chr
return result
end
The tests fail, and here’s what one looks like:
2) Failure:
test_A_D(TestDiamond) [/Users/ron/Programming/diamond/diamondtest.rb:65]:
<"--B-B--"> expected but was
<"--BAB--">.
Sure enough, the string is being reused. We need to copy it. Doubtless Ruby knows how to do that. Checking the Pickaxe … there is clone and dup … those might do it. No, the thing is to do String.new(string_to_copy). That even has some sexy behavior of not actually copying until the thing is changed. Anyway it should do the job for us.
def maker_right(k)
result = String.new(@base)
result[k] = (k+65).chr
return result
end
That does the job perfectly. Also it leaves us staring at that cryptic (k+65).chr. The 65 is the numeric value of the letter ‘A’. That’s ugly. Let’s do the string lookup trick again here.
def maker_right(k)
result = String.new(@base)
result[k] = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'[k]
return result
end
Sweet! That works. Does it seem odd that the k-th position in our dashed string contains the k-th letter of the alphabet? It did to me, but of course we’re starting from the center ‘A’ and progressing one letter at a time to the right, so it makes sense.
Thinking …
Remember, we’re thinking all the time. Mostly – almost entirely – I’m just looking at the code and thinking what it does and how to make it better. I’m not really thinking about the diamond problem at all now. My understanding of diamond is in the code, and I’m improving the code.
That maker_right function copies some dashes, slams in a letter, returns the result. I wonder if there is some clever String thing one could do that all in one go, or at least to reduce it to two lines. Well, after a bit of scanning and rumination, I don’t see anything that I like better. The code there is clear and anything else is likely to be more cryptic. Maybe some Ruby wizard will drop me an email and tell me what I should have done.
The code now
Let’s look at the whole program again. Skip over it if you prefer.
require 'test/unit'
class DiamondMaker
attr_reader :stop_integer
def initialize(stop)
@stop_integer = stop.bytes[0] - 'A'.bytes[0]
@base = '-------------------------------------'[(0..@stop_integer)]
end
def maker_right(k)
result = String.new(@base)
result[k] = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'[k]
return result
end
def full_line(k)
base = maker_right(k)
slice = base[1, base.length]
return slice.reverse + base
end
def diamond
base = (0..@stop_integer).collect { |i| self.full_line(i)}
slice = base[0,base.length-1]
return base + slice.reverse
end
end
class TestDiamond < Test::Unit::TestCase
def test_hookup
assert_equal(3, 2+1)
end
def test_counts
maker = DiamondMaker.new('D')
assert_equal(3, maker.stop_integer)
end
def test_A
maker = DiamondMaker.new('A')
diamond = maker.diamond
assert_equal(1, diamond.length)
assert_equal('A', diamond[0])
end
def test_A_C_full
maker = DiamondMaker.new('C')
b_line = maker.full_line(1)
assert_equal('-B-B-', b_line)
end
def test_A_B_left
maker = DiamondMaker.new('B')
assert_equal('A-', maker.maker_right(0))
assert_equal('-B', maker.maker_right(1))
end
def test_A_D
maker = DiamondMaker.new('D')
diamond = maker.diamond
assert_equal(7, diamond.length)
assert_equal('---A---', diamond[0])
assert_equal('--B-B--', diamond[1])
assert_equal('-C---C-', diamond[2])
assert_equal('D-----D', diamond[3])
assert_equal('-C---C-', diamond[4])
assert_equal('--B-B--', diamond[5])
assert_equal('---A---', diamond[6])
end
end
That’s pretty good but I’m concerned about this:
def diamond
base = (0..@stop_integer).collect { |i| self.full_line(i)}
slice = base[0,base.length-1]
return base + slice.reverse
end
The names aren’t good. The first line computes the top of the diamond. We should call it top or something. The second line is getting the “tip” of the diamond or all but the center line. Needs improvement. Let’s try this:
def diamond
top = (0..@stop_integer).collect { |i| self.full_line(i)}
bottom = top[0,top.length-1].reverse
return top + bottom
end
Yes, I like that better. There’s that length-1 in there. And isn’t @stop_integer a lot like length. This calls for a range, this time non-inclusive:
def diamond
top = (0..@stop_integer).collect { |i| self.full_line(i)}
bottom = top[(0...@stop_integer)].reverse
return top + bottom
end
Nice, I like that. You and your team might disagree: the difference between .. and ... is pretty subtle. But my team likes it.
More of that thinking thing
The code for top collects all the top rows into an array. The code to get the row is called full-line. Maybe it should be called row. But that’s not where I was going. The code to compute a row, together with diamond looks like this:
def full_line(k)
base = maker_right(k)
slice = base[1, base.length]
return slice.reverse + base
end
def diamond
top = (0..@stop_integer).collect { |i| self.full_line(i)}
bottom = top[(0...@stop_integer)].reverse
return top + bottom
end
These are doing very similar things. The full_line is like the “bottom” of the diamond, the tail A--- part, prepended with the slice representing the top. There’s duplication here. We must consider rooting it out.
Aside …
I once saw Kent Beck look at two patches of code that were even more different than these two are and declare that he saw duplication. He made the two patches even more similar and then extracted the duplication, improving the code. I’m no Kent Beck, by a long shot, but I do think something could be done here
Therefore …
What if we reversed the maker_right line and called it left? Then our slice could be called right. Let’s try that:
def full_line(k)
left = maker_right(k).reverse
right = left[0, left.length-1].reverse
return left + right
end
This works. And we see in the left subscripting that we have the opportunity to use the non-inclusive range again:
def full_line(k)
left = maker_right(k).reverse
right = left[(0...@stop_integer)].reverse
return left + right
end
def diamond
top = (0..@stop_integer).collect { |i| self.full_line(i)}
bottom = top[(0...@stop_integer)].reverse
return top + bottom
end
These are quite close. The bottom two lines are in fact duplication now. Extract method:
def full_line(k)
left = maker_right(k).reverse
reflect(left)
end
def diamond
top = (0..@stop_integer).collect { |i| self.full_line(i)}
reflect(top)
end
def reflect(array)
array + array[(0...@stop_integer)].reverse
end
Nota Bene, as Sister Mary Carmelita used to say
The fact that the result is symmetrical about x and y is “well-known”. Our code did not reflect that, no pun intended. Now it does.
The significant thing relating to Alistair’s recommendation about thinking up front is that the expression of the symmetry came about by thinking concretely about the code, not thinking abstractly about the problem. Better? I think so. But certainly different, with similar results. Does his code exploit the two-way symmetry at all? I haven’t looked.
Moving on …
The name maker_right is pretty terrible. It should be something about “right side” and if we reversed it inside the method, “left side”.
The diamond method isn’t good. It includes a detailed calculation and a function call. That’s lopsided. We should used Composed Method pattern and pull out the collect. It would be named something about “top” and it would be nice if the names for top and left were similar. What about left and top? We’d have to rename some locals to do that but it might be worth it. Here goes:
def left(k)
result = String.new(@base)
result[k] = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'[k]
return result.reverse
end
def full_line(k)
left_side = left(k)
reflect(left_side)
end
That’s a bit nicer. I had to fiddle the test that looked for maker_right, and I don’t entirely like what happened:
def test_A_B_left
maker = DiamondMaker.new('B')
assert_equal('A-', maker.left(0).reverse)
assert_equal('-B', maker.left(1).reverse)
end
I should have reversed the test strings, but I felt reversing the answer back was easier. Bad Ron. Make a note of that for later.
The name full_line isn’t great. How about row?
def left(k)
result = String.new(@base)
result[k] = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'[k]
return result.reverse
end
def row(k)
left_side = left(k)
reflect(left_side)
end
def diamond
top = (0..@stop_integer).collect { |i| self.row(i)}
reflect(top)
end
Yes, that’s better. Let’s extract that collect into a method now:
def top
(0..@stop_integer).collect { |i| self.row(i)}
end
def diamond
top_part = top
reflect(top)
end
Do you see my mistake? I stored top in top_part but reflected top. And it worked, of course. We can remove the “explaining variable”, and do similarly in the row method as well:
def left(k)
result = String.new(@base)
result[k] = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'[k]
return result.reverse
end
def row(k)
reflect(left(k))
end
def top
(0..@stop_integer).collect { |i| self.row(i)}
end
def diamond
reflect(top)
end
def reflect(array)
array + array[(0...@stop_integer)].reverse
end
Now that’s interesting, isn’t it? All the operations have come down to one line of code per method, except for left.
My initial reaction on looking at that is that it is no longer as clear as it was. The reason, I think, is that the local variables were doing a bit of explaining of what was going on. I’m inclined to put them back, improving the names, rather than leave it here.
But before we do …
It’s worth noting how compact this code can be made, all by reasoning about the code, with little or no thinking up front. And we’re sure it works because we have our tests.
After renaming and explaining variables
I added explaining variables, renamed some functions, and normalized the use of k and i throughout. Here’s the code now:
require 'test/unit'
class DiamondMaker
attr_reader :stop_integer
def initialize(stop)
@stop_integer = stop.bytes[0] - 'A'.bytes[0]
@base = '-------------------------------------'[(0..@stop_integer)]
end
def left_side_of_row(i)
result = String.new(@base)
result[i] = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'[i]
return result.reverse
end
def row(i)
left_side = left_side_of_row(i)
reflect(left_side)
end
def top_of_diamond
(0..@stop_integer).collect { |i| self.row(i)}
end
def diamond
top_part = top_of_diamond()
reflect(top_part)
end
def reflect(array)
array + array[(0...@stop_integer)].reverse
end
end
class TestDiamond < Test::Unit::TestCase
def test_hookup
assert_equal(3, 2+1)
end
def test_counts
maker = DiamondMaker.new('D')
assert_equal(3, maker.stop_integer)
end
def test_A
maker = DiamondMaker.new('A')
diamond = maker.diamond
assert_equal(1, diamond.length)
assert_equal('A', diamond[0])
end
def test_A_C_full
maker = DiamondMaker.new('C')
b_line = maker.row(1)
assert_equal('-B-B-', b_line)
end
def test_A_B_left
maker = DiamondMaker.new('B')
assert_equal('-A', maker.left_side_of_row(0))
assert_equal('B-', maker.left_side_of_row(1))
end
def test_A_D
maker = DiamondMaker.new('D')
diamond = maker.diamond
assert_equal(7, diamond.length)
assert_equal('---A---', diamond[0])
assert_equal('--B-B--', diamond[1])
assert_equal('-C---C-', diamond[2])
assert_equal('D-----D', diamond[3])
assert_equal('-C---C-', diamond[4])
assert_equal('--B-B--', diamond[5])
assert_equal('---A---', diamond[6])
end
end
Very TL;DR by now, I’m sure
What has happened here? I think the main point is that we were quite able to develop a solution that makes “mathematical sense”, including discovering and exploiting the left-right and top-bottom symmetry of the solution. We did so with very little thinking up front.
There was some thinking about the problem, though as I recall it, even there I was mostly thinking about the string I wanted to get back.
The bulk of the work was in improving our “proof”, the reasoning that gets us the answer. In many cases, even more than I reported here, the tests caught me in a typo or logic error.
Somewhere up there, I wrote (0..@stop_integer) when I should have used three dots. The tests exploded and I just typed in the extra dot. I apologize if I didn’t mention it before.
I simplified the code too far for my taste, recognized that fact, and revised it to be more clear. It may or may not be the way you would do it, and your job is not to do what I do, but to observe what I do and use the parts of it that make sense to you. Your code should be different, as should your usage of TDD.
You should decide for yourself how much up front thinking to do. It’s cheap, and fun, if done within reason. You might want to consciously push back against up front thinking, moving to code sooner, and pushing more on thinking as you go.
Or you might just want a nap. That’s good too.
Thanks for reading …