Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm working through some exercises to sharpen my Ruby skills. The following code is how I solved this particular question. I'd like to know what other developers think of this solution. I'm self taught, which means: I'm not getting any feedback as I learn, so I just want to make sure that I'm learning the right way to do things. And I'd like to get a gauge of what professional developers think of my code. I appreciate all your feedback.

Ranking System
[1-terrible] [2-not good] [3-ok] [4-good] [5-very good]

    1. Is the answer code elegant? [score and feedback]
    2. What tips can you suggest to improve the answer?
    3. How's the readability of the code? [score and feedback]
    4. Can you suggest a simpler and alternative answer?

My Ruby Code

class Book
  def title
    @title
  end

  def title=(title)
  special_words = %w(and in the of a an)
  formatted_title = []

    @title = title.split.each_with_index do |w,i|
      case 
      when i == 0
        formatted_title << w.capitalize

      when i > 0 && !special_words.include?(w)
        formatted_title << w.capitalize

      when special_words.include?(w)
        formatted_title << w
      end
    end

  @title = formatted_title.join(" ")
  end
end

Rspec

describe Book do

  before do
    @book = Book.new
  end

  describe 'title' do
    it 'should capitalize the first letter' do
      @book.title = "inferno"
      @book.title.should == "Inferno"
    end

    it 'should capitalize every word' do
      @book.title = "stuart little"
      @book.title.should == "Stuart Little"
    end

    describe 'should capitalize every word except...' do
      describe 'articles' do
        specify 'the' do
          @book.title = "alexander the great"
          @book.title.should == "Alexander the Great"
        end

        specify 'a' do
          @book.title = "to kill a mockingbird"
          @book.title.should == "To Kill a Mockingbird"
        end

        specify 'an' do
          @book.title = "to eat an apple a day"
          @book.title.should == "To Eat an Apple a Day"
        end
      end

      specify 'conjunctions' do
        @book.title = "war and peace"
        @book.title.should == "War and Peace"
      end

      specify 'prepositions' do
        @book.title = "love in the time of cholera"
        @book.title.should == "Love in the Time of Cholera"
      end
    end

    describe 'should always capitalize...' do
      specify 'I' do
        @book.title = "what i wish i knew when i was 20"
        @book.title.should == "What I Wish I Knew When I Was 20"
      end

      specify 'the first word' do
        @book.title = "the man in the iron mask"
        @book.title.should == "The Man in the Iron Mask"
      end
    end
  end
end
share|improve this question
    
i'd say specs -- 4, code -- 3. i'd answer more fully but I don't have anything to add to Phillip's solution, which I like a lot other than the word "titlieze" –  Jonah Mar 4 at 3:23
    
@Jonah Thanks for your input. I agree about "titlieze" –  fab Mar 4 at 3:25
    
I use this tool for my blog to auto-capitalize titles, but if you look at the source code, you can see how they wrote it in javascript following the Chicago Manual of Style rules for title capitalization. –  user57018 Oct 28 at 12:05

3 Answers 3

up vote 8 down vote accepted

Your code is readable, but it doesn't feel very "rubyish" to me. I think that's due to needing to setup and track some extra variables and track the index of the word in question -- having to case it depending on the index, etc.

It also bugs me a little bit that title= is doing so much. This is how I would have written it:

class Book
  def title
    @title
  end

  def title=(title)
    @title = titlieze(title)
  end

  private
  def titlieze(title)
    stop_words = %w(and in the of a an)
    title.capitalize.split.map{|w| stop_words.include?(w) ? w : w.capitalize}.join(' ')
  end

end

I used stop_words instead of special_words since that's a common naming scheme for search applications (ie. words you ignore when searching). I'd be tempted to move that into a constant or some other configuration, but it works fine for this.

Rspec output:

$ rspec -f d foo_spec.rb

Book
  title
    should capitalize the first letter
    should capitalize every word
    should capitalize every word except...
      conjunctions
      prepositions
      articles
        the
        a
        an
    should always capitalize...
      I
      the first word

Finished in 0.00261 seconds
9 examples, 0 failures
share|improve this answer
    
It does? Did you change your test from when it was on stackoverflow? It's passing on my system using Ruby 2. –  Philip Hallstrom Mar 3 at 20:39
    
Hey thanks. Sorry about that, I made a mistake copying your code. It works well and is very nice. Thank you for your feedback. +1 –  fab Mar 3 at 20:42
    
How does it know to always capitalize the first word (regardless) if it is a stop word? –  fab Mar 3 at 20:44
    
Actually I see .. It's because you call it as: title.capitalize... –  fab Mar 3 at 20:54

Your indentation is a bit inconsistent, making the code hard to read.

This short method

def title
  @title
end

… is commonly written using meta-programming.

The title= method could be improved by shortening it. There is too much reassignment going on: making an empty formatted_title, appending each word to it, then setting @title, but — just kidding! — we still have to re-join the words into a string! Better to do the job right the first time.

I also think that the code would be easier to understand by collapsing the three cases into two. Then you could easily state exactly when a word should be capitalized.

class Book
  attr_reader :title

  # Define this constant array just once
  @@SPECIAL_WORDS = %w(and in the of a an)

  def title=(title)
    @title = title.split.each_with_index.map do |w,i|
      case 
        when i == 0 || !@@SPECIAL_WORDS.include?(w)
          # Capitalize the first word and all subsequent non-special words
          w.capitalize
        else
          w
      end
    end.join(' ')
  end
end

Consider leaving words that already have internal capitalization unchanged (e.g. "iPhone").

Also consider that you might already have a String.titleize() if you're using ActiveSupport.

share|improve this answer
    
Whats the reason for creating a class variable constant? Are you just assuming that the special words may need to available to other methods on the Book class? –  fab Mar 3 at 20:50
    
Just to emphasize that it never changes. I don't feel strongly about that particular piece of advice. –  200_success Mar 3 at 20:56
    
So you're saying a constant within the method itself may suffice? –  fab Mar 3 at 20:58
    
Sure, that could work too. –  200_success Mar 3 at 22:59

Though I am not a professional developer, I will venture one suggestion: learn how to use String methods and regexes to fullest advantage. Sometimes it is necessary to use split, partition, etc. to convert a string to an array of strings (possibly single-character strings), manipulate the array elements, then rejoin them into a string, but there is a lot you can do by working on the string directly. Here, for example, you can use String#gsub with a block:

str = "to eat an apple a day" 
sw = %w[and in the of a an]

str.capitalize.gsub( /\S+/ ) { |w| sw.include?(w) ? w : w.capitalize }
  #=> "To Eat an Apple a Day"

Edit: initially I had ...gsub( /\w+/ ).... I am grateful to @feed_me_code for the suggestion to use \S rather than \w.

share|improve this answer
1  
+1 @Cary your suggestion: "learn how to use String methods and regexes to fullest advantage" is great. I wanted to add that instead of \w+ boundary.. Consider \S Any non-whitespace character. This takes into account strings with punctuation. Consider this: str = "the girl's voyage back to earth" Thanks again for your advice. –  fab Mar 6 at 19:43
    
Good point, feed. Will edit. –  Cary Swoveland Mar 7 at 6:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.