Skip to content

Commit

Permalink
first kata (Osherove TDD 1)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jredville
jredville committed May 9, 2012
1 parent 17973b6 commit 67d0010
Showing 1 changed file with 80 additions and 0 deletions.
80 changes: 80 additions & 0 deletions osherove-tdd-1-05-09-12.rb
View file
@@ -0,0 +1,80 @@
# TDD Kata from http://osherove.com/tdd-kata-1/
module StringCalculator
extend self

DEFAULT_DELIMITER = ","
def add(str)
res = split_str(str)
res = convert_items(res)
res.inject(0, &:+)
end

def parse_delimiter(str)
delimiter = DEFAULT_DELIMITER
lines = str.split("\n")

flags = lines.first
if flags && flags[0..1] == "//"
delimiter = lines.shift[2..-1]
end
return delimiter, lines
end

def split_str(str)
delimiter, lines = parse_delimiter(str)
lines.map {|line| line.split(delimiter)}.flatten
end

def convert_items(arr)
arr.map(&:to_i)
end
end

# Notes from after kata
# * Made it to step 4
# * feels ugly....
# * the overall system doesn't feel natural, it feels forced
# * procedural, not object oriented
# * no tests for my helper methods. I considered them as
# private when I did it, but question that now
# * not obvious from the final result, but lots of churn here
# * inconsistent refactoring of "magic values"
# *

require 'rubygems'
require 'bacon'

describe "String calculator" do
it 'has the add method' do
StringCalculator.method(:add).should.not.equal nil
end

it 'does not have 0 arity' do
StringCalculator.method(:add).arity.should.equal 1
end

it 'returns 0 for empty string' do
StringCalculator.add("").should.equal 0
end

it 'returns 1 for "1"' do
StringCalculator.add("1").should.equal 1
end

it 'adds 1 and 2 for "1,2"' do
StringCalculator.add("1,2").should.equal 3
end

it 'adds multiple numbers' do
StringCalculator.add("1,2,3").should.equal 6
end

it 'can handle newline delimiters' do
StringCalculator.add("1\n2").should.equal 3
end

it 'can support new delimiters' do
StringCalculator.add("//;\n1;2").should.equal 3
end
end

7 comments on commit 67d0010

@timlinquist
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In response to your procedural comment, it would be easy for you to break this down even further ! After ruminating on the functions, the code is yelling at me to have a Parser! It literally yelled at me! Then your StringCalculator would just be a string calculator. The benefit here would be a couple things

  • Reuse of the parser outside of calculating things
  • Simplifying the string calculator (might just be the same benefit, srp.)

Then we could make the StringCalculator's internals all kinds of beautiful:

   class Parser
    ...code omitted
   end 

  module StringCalculator
    extend self #a bit too clever for me as the intent is not immediately apparent

    def add(string)
      parse(string) { |numbers|  number.inject(&:+) }
    end

    def parser
      @parser ||= Parser.new
    end

    def parse &block
      yield parser.parse @string
    end
  end

We could clean up this jot further too. Moving the #parse method to the parser then delegating would get rid of some redundant code for instance. This would make adding additional functions to the calculator trivial.

Great job throwing this out there for people to give feedback on. What do you think about what I think!?!

@reinh
Copy link

@reinh reinh commented on 67d0010 May 9, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jim, I'm one of Tim's coworkers.

One of the ways I like to look at OO design is by trying to identify as many nouns in the domain as possible. Verbs are very procedural ("do this", "compute that") whereas nouns are a more natural fit for a programming paradigm built around objects. (Also note that a module with only module methods is de facto procedural code). For starters, we need to create a StringCalculator object to hold our data and the behavior we want to attach to it.

class StringCalculator
  def initialize(input)
    @input = input
  end
end

Now that we've defined our domain, what kind of nouns should it have? Well, instead of parse_delimiter and split_str and so on, what is the most basic noun in an adding machine? It's "numbers", the collection of numbers to be added, right? So why not build a numbers method? Then your adding is simply numbers.reduce :+, which is very intention revealing.

So what would that numbers method have to do? Well, it would have to parse things and split things and so on, but again if you use a vocabulary of nouns I think the design will be easier to suss out. For instance, why not have a delimiter noun? You have a variable named delimiter, but it's hidden away inside an opaque data structure (the [delimiter, lines] tuple). Here's a naive implementation. Good thing you have tests!

def delimiter
  @input[%r|^//(.)|][2]
end

What other nouns would be helpful? What about lines, the input split into lines and with any delimiter specification removed.

def lines
  @input.sub(%r|^//(.)|, '').split("\n")
end

And perhaps now we are ready to write numbers?

def numbers
  lines.map{|line| line.split(delimiter)}.flatten.map(&:to_i)
end

And then of course add, the command that makes this a Command Object:

def add
  numbers.reduce :+
end

def self.add(input)
  new(input).add
end

Does this work? No idea. That's what your tests are for. But I hope the ideas are useful.

@timlinquist
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3 <3 magnifique. @jredville you just invented distributed red, green, refactor, review, refactor !

@reinh
Copy link

@reinh reinh commented on 67d0010 May 9, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@jredville
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timlinquist: The parser idea is great. I considered it, but then ignored the idea as overkill for this problem. Looking at it now, and at your example, it is obvious that the parser is a separate responsibility, thus should be separate. I think it would cause the entire algorithm to just fall into place.

As for "distributed red, green, refactor, review, refactor" - ❤️ it!

@reinh: Hi Rein, don't know if you remember, but we met at Ruby on Ales this year and hung out a little at Deschutes. Thanks for your comments, there are some really good points in there. I'm really digging the idea of defining the domain by finding the nouns, I think it would probably help both on the design side, and the naming side. I also like the approach you took of building up the domain part by part. I often do that, but I think I neglected it due to the list that was sitting in front of me when I approached this Kata. Your point about module methods is a good one too; it is probably smart to be suspicious of modules with behavior that aren't being mixed in!

I'm really looking forward to trying this again tomorrow with these ideas to help shape it.

@reinh
Copy link

@reinh reinh commented on 67d0010 May 10, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jredville I'm pretty bad at placing internet names with faces, sorry!

I did do this in a bottom up fashion but it should be pretty easy to arrive at a similar structure by writing #numbers and then extracting methods:

def numbers
  delimiter = @input[%r|^//(.)|][2]
  lines = @input.sub(%r|^//(.)|, '').split("\n")
  lines.map{|line| line.split(delimiter)}.flatten.map(&:to_i)
end

The temporary explaining variables then become obvious targets for an extract method refactoring. Or, as the code is already pretty intention revealing, you may even leave as is.

@jredville
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reinh No prob. If we were drinking beers weekly I'd be offended :)

That path makes sense as well. I ended up using the regex method you use here in my latest version (cdcaf35), and found it feels better than the method of parsing the delimiter out as I go, i.e.

  def lines
    @input.split("\n")
  end

  # this is more to show technique than style... it's ugly as hell
  def cleaned_lines
    cleaned = lines
    flagged_line = cleaned[0]
    if flagged_line && flagged_line[0..1] == "//"
      @delimiter = flagged_line[2..-1]
      cleaned.unshift
    end
    cleaned
  end

Please sign in to comment.