2
\$\begingroup\$

I just finished the book title class exercise for The Odin Project and I would appreciate some honest feedback for my code. The code is intended to be "test-driven learning", and therefore emulates test driven development

Here is the test spec:

require 'book'

describe Book do

  before do
    @book = Book.new
  end

  describe 'title' do
    it 'should capitalize the first letter' do
      @book.title = "inferno"
      expect(@book.title).to eq("Inferno")
    end

    it 'should capitalize every word' do
      @book.title = "stuart little"
      expect(@book.title).to eq("Stuart Little")
    end

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

        specify 'a' do
          @book.title = "to kill a mockingbird"
          expect(@book.title).to eq("To Kill a Mockingbird")
        end

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

      specify 'conjunctions' do
        @book.title = "war and peace"
        expect(@book.title).to eq("War and Peace")
      end

      specify 'prepositions' do
        @book.title = "love in the time of cholera"
        expect(@book.title).to eq("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"
        expect(@book.title).to eq("What I Wish I Knew When I Was 20")
      end

      specify 'the first word' do
        @book.title = "the man in the iron mask"
        expect(@book.title).to eq("The Man in the Iron Mask")
      end
    end
  end
end

Here if my code:

class Book
  attr_accessor :title

  def title=(book)
    lowercase = ["and", "or", "the", "of", "in", "a", "an"]
    @title = book.capitalize.split(" ").map do |word|
      if lowercase.include? word
        word
      else
        word.capitalize
      end
    end.join(" ")
  end

end
\$\endgroup\$
1
\$\begingroup\$

Your code looks good. But I have some suggestions. lowercase array will be created on each method call, so we can just declare it as class level constant (and use %w() syntax):

LOWERCASE = %w(and or the of in a an).freeze

Try to use single quotes if possible.

It is possible to use next inside map block to avoid if/else. And split(' ') is the same as split.

Regarding specs. There's neat RSpec feature called shared_examples(RSpec shared_example).

Here's my suggested refactoring:

class Book
  attr_accessor :title

  LOWERCASE = %w(and or the of in a an).freeze

  def title=(book)
    @title = book.capitalize.split.map do |word|
      next word if LOWERCASE.include?(word)
      word.capitalize
    end.join(' ')
  end

end

RSpec.shared_examples 'capitalize' do |message, text, title|
  let!(:book) { described_class.new }
  let!(:assign_title) { book.title = text }

  it message do
    expect(subject).to eq(title)
  end
end

RSpec.describe Book do
  context 'title' do
    subject { book.title }

    it_behaves_like 'capitalize', 'first letter', 'inferno', 'Inferno'
    it_behaves_like 'capitalize', 'every word', 'stuart little', 'Stuart Little'

    context 'articles' do
      it_behaves_like 'capitalize', 'except "the"', 'alexander the great', 'Alexander the Great'
      it_behaves_like 'capitalize', 'except "a"', 'to kill a mockingbird', 'To Kill a Mockingbird'
      it_behaves_like 'capitalize', 'except "an"', 'to eat an apple a day', 'To Eat an Apple a Day'
    end

    it_behaves_like 'capitalize', 'except conjunctions', 'war and peace', 'War and Peace'
    it_behaves_like 'capitalize', 'except prepositions', 'love in the time of cholera', 'Love in the Time of Cholera'
    it_behaves_like 'capitalize', 'always for "I"', 'what i wish i knew when i was 20', 'What I Wish I Knew When I Was 20'
    it_behaves_like 'capitalize', 'always for first word', 'the man in the iron mask', 'The Man in the Iron Mask'
  end
end
\$\endgroup\$
0
\$\begingroup\$

I'm not a big fan of using capitalize on the whole string to ensure that first word stays capitalized. I'd really rather add && index != 0 to the condition to express is clearly, although I'm not sure that everyone would agree with that on me.

Other than that, your code is fine.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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