Ruby Kata Gem: WordWrap

Question

I've just completed the WordWrap Kata in the Ruby Kata gem and I'd love some feedback. Thanks!

Here's the Kata:

$kata take word_wrap.rb
WordWrap Kata
   Create a "WordWrap" class that is initialized with an integer parameter
      - detail: The parameter is the wrap column
      - example: WordWrap.new 4
      - detail: Non-integer parameters raise an exception
      - example: WordWrap.new raises an exception
      - example: WordWrap.new "foo" raises an exception

completed (Y|n):

   "wrap" method
      Create a wrap method that takes a string parameter
         - example: WordWrap.new(5).wrap "this is text"
         - detail: The method should raise an exception for non-string parameters
         - example: WordWrap.new(4).wrap([1,2,3]) raises an exception

completed (Y|n):

      It should wrap the empty string to an empty string
         - example: WordWrap.new(4).wrap("") returns ""

completed (Y|n):

      It should return one short word as-is
         - example: WordWrap.new(6).wrap("word") returns "word"

completed (Y|n):

      It should wrap text at the last space before the wrap length
         - example: WordWrap.new(5).wrap("word word") returns "word\nword"
         - example: WordWrap.new(5).wrap("word word word") returns "word\nword\nword"
         - example: WordWrap.new(10).wrap("word word word") returns "word word\nword"

completed (Y|n):

      Long words without spaces should be broken at the wrap length
         - example: WordWrap.new(4).wrap("wordword") returns "word\nword"

completed (Y|n):

      Strings with long words and spaces should mix breaking words and at spaces
         - example: WordWrap.new(3).wrap("word word word") returns "wor\nd\nwor\nd\nwor\d"

completed (Y|n):

┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━┓
┃ Requirement                                                                      ┃ Time     ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Create a "WordWrap" class that is initialized with an integer parameter          ┃ 00:09:37 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Create a wrap method that takes a string parameter                               ┃ 00:06:59 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ It should wrap the empty string to an empty string                               ┃ 00:03:55 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ It should return one short word as-is                                            ┃ 00:01:53 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ It should wrap text at the last space before the wrap length                     ┃ 00:34:53 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Long words without spaces should be broken at the wrap length                    ┃ 00:06:16 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Strings with long words and spaces should mix breaking words and at spaces       ┃ 00:05:04 ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┻━━━━━━━━━━┛

My specs:

require 'spec_helper'
require 'wordwrap'

describe WordWrap do
  describe "#initialize" do
    let(:wrap_column) { 1 }

    it "instantiates" do
      expect {
        WordWrap.new
      }.to raise_exception
    end
    it "accepts an integer parameter" do
      expect { WordWrap.new(wrap_column) }.to_not raise_exception
    end
    it "rejects non integer parameters" do
      expect { WordWrap.new("foo") }.to raise_exception
    end
  end
  describe ".wrap" do
    let(:wrap_column) { 5 }
    subject(:word_wrap) { WordWrap.new(wrap_column) }
    specify { expect { word_wrap.wrap("this is text") }.to_not raise_exception }
    specify { expect { word_wrap.wrap([1,2,3]) }.to raise_exception }
    it "wraps the empty string to the empty string" do
      word_wrap.wrap("").should eq("")
    end
    it "returns short words (i.e. char count < wrap_column) without modifying them" do
      word_wrap.wrap("word").should eq("word")
    end
    it "wraps text at the last space before the wrap_column" do
      WordWrap.new(5).wrap("word word").should eq("word\nword")
    end
    it { WordWrap.new(5).wrap("word word word").should eq("word\nword\nword") }
    it { WordWrap.new(10).wrap("word word word").should eq("word word\nword") }
    it { WordWrap.new(4).wrap("wordword").should eq("word\nword") }
    it "mixes breaks at words and spaces" do
      WordWrap.new(3).wrap("word word word").should eq("wor\nd\nwor\nd\nwor\nd")
    end
  end
end

And, finally, the WordWrap Class:

class WordWrap
  def initialize(wrap_column)
    if (!wrap_column.integer?)
      raise(ArgumentError, "expected Integer got #{wrap_column.class}")
    end
    @wrap_column = wrap_column
  end

  def wrap(words)
    if (!words.is_a?(String))
      raise(ArgumentError, "expected String and got #{words.class}")
    end

    wrapped_words = ""
    wrap_count = 0
    word_array = words.scan(/\w+/)

    word_array.each_with_index do |current_word, index|
      next_word = word_array[index + 1]
      puts "current_word: #{current_word}"
      puts "next_word: #{next_word}"

      if (current_word.length > @wrap_column)
        wrapped_words << current_word[0..@wrap_column - 1] << "\n"
        wrapped_words << current_word[@wrap_column..-1]
        if !next_word.nil?
          wrapped_words << "\n"
        end
      elsif next_word.nil?
        wrapped_words << current_word
      else
        if (current_word.length + next_word.length + wrap_count) > @wrap_column
          wrapped_words << current_word << "\n"
          wrap_count = 0
        else
          wrapped_words << current_word << " "
          wrap_count += current_word.length + next_word.length
        end
      end
    end
    wrapped_words
  end
end

As you can see, wrap() is a pretty ugly function with lots of branching. Any ideas for streamlining it would be greatly appreciated.

Edit:

Thanks to the suggestions below, I've refactored wrap into something more readable:

def wrap text
    if (text.class != String)
      raise Exception "expected String but paramater has class #{text.class}"
    end

    if text.empty?
      return ""
    end

    letter_array = text.split("")
    last_space = false
    offset = 0
    current_length = 0

    letter_array.each_with_index do |letter, index|
      if (letter =~ /\s/)
        last_space = index
      end
      if (current_length == @wrap_length && last_space)
        letter_array[last_space] = "\n"
        current_length = index - last_space
        last_space = false
      elsif (current_length == @wrap_length)
        letter_array.insert(index, "\n")
        current_length = 0
      else
        current_length += 1
      end
    end

    if (letter_array.last == "\n")
      letter_array.delete_at(-1)
    end
    letter_array.inject(:+)
  end

Answer 1

Here are some things you can improve:

1. Ruby has a method each_cons(n). In your code, you can write word_array.each_cons(2) do |current_word, next_word|.
2. There is a bug in the code where you write wrapped_words << current_word[@wrap_column..-1]. This assumes that a word can be broken only once. For example, WordWrap.new(3).wrap("wordword") will return "wor\ndword".
3. Extending the above logic, the next block adding a new line in the case next_word is not nil is also a bug. Consider the input "word w" when you have to wrap on 3 columns. Your output will be "wor\nd\nw" whereas the correct output should be "wor\nd w".

I'll not be adding any more code comments since you seem to have some pretty serious bugs in your code owing to all those conditions. What I do recommend is to revise your strategy. Try thinking along these lines for a pretty solution:

1. Scan the input string one character at a time.
2. Remember the current length of scan.
3. Remember the position of the last space seen every time you encounter a space.
4. If the current length of the scan is equal to the wrap value, it's time to take action based on the position of the last space seen:
   1. If it's not nil, you have to copy everything up to that position into the result string plus a new line. Reset the current_length based on how much was copied.
   2. If it's nil, copy everything up to the current index and then add a new line.
5. Reset the position of the last space seen to nil.
6. If it's the end of string and your result string has a new line at the end, chomp it off.