0
\$\begingroup\$

I have been learning Ruby for 2 weeks. Today my bootcamp teacher taught Ruby Class and gave me homework. I coded simple library program. But I think the code is smelling bad. How can do better the code?

 class Book
  attr_reader :book_name, :author_name, :page_count, :more, :book_array
  def initialize()
    @book_name = ''
    @author_name = ''
    @page_count = 0
    @more = 'yes'
    @book_array=[]
  end

  def answers_start
    until more_book?
      ask
      add_arr
    end

    book_list

  end

  private

  def ask
    puts 'Name of book'
    @book_name=gets.chomp
    puts 'Author of the book'
    @author_name=gets.chomp
    puts 'Page count of the book'
    @page_count=gets.to_i
    puts 'Do you want add more book'
    @more=gets.chomp.capitalize
  end

  def more_book?
    @more == 'No'
  end

  def book_list
    @book_array.each_with_index do |item,index|
      puts "Book#{index+1} {Name: #{item[:book_name]}, Author: #{item[:author_name]}, Page count: #{item[:page_count]}}"
    end
  end

  def add_arr
    book_array << {book_name:@book_name, author_name:@author_name, page_count:@page_count}
  end

end



books = Book.new()
books.answers_start
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Consistency

Sometimes you use accessor methods, sometimes you access instance variables directly. Sometime you use whitespace around operators, sometimes you don't. Sometimes you use space after a comma, sometimes you don't. Sometimes you use an empty argument list and sometimes you use no argument list at all when you send a message with no arguments. Sometimes you use an empty parameter list and sometimes you use no parameter list at all when you define a method with no parameters.

You should choose one style and stick with it. If you are editing some existing code, you should adapt your style to be the same as the existing code. If you are part of a team, you should adapt your style to match the rest of the team.

Most communities have developed standardized community style guides. In Ruby, there are multiple such style guides. They all agree on the basics (e.g. indentation is 2 spaces), but they might disagree on more specific points (single quotes or double quotes).

In general, if you use two different ways to write the exact same thing, the reader will think that you want to convey a message with that. So, you should only use two different ways of writing the same thing IFF you actually want to convey some extra information.

For example, some people always use parentheses for defining and calling purely functional side-effect free methods, and never use parentheses for defining and calling impure methods. That is a good reason to use two different styles (parentheses and no parentheses) for doing the same thing (defining methods).

Indentation

The community standard for indentation is 2 spaces, and there should be no indentation at the beginning of the script. You are starting out with an indentation of 1 space, and the next line is again indented 1 space:

 class Book
  attr_reader :book_name, :author_name, :page_count, :more, :book_array

Instead, class Book should have no indentation at all, and attribute_reader should be indented 2 spaces relative to class Book.

Like this:

class Book
  attr_reader :book_name, :author_name, :page_count, :more, :book_array

Whitespace around operators

There should be 1 space either side of an operator. You sometimes use 1 space and sometimes no space.

For example here you are using two different styles literally in two consecutive lines:

@more = 'yes'
@book_array=[]

This should be

@more = 'yes'
@book_array = []

Space after comma

There should be 1 space after a comma. You sometimes use 1 space, sometimes no space.

For example, this:

@book_array.each_with_index do |item,index|

should be this:

@book_array.each_with_index do |item, index|

Space after colon in a hash literal

There should be 1 space after the colon in a hash literal.

For example, this:

book_name:@book_name

should be this:

book_name: @book_name

Space in a hash literal

There should be 1 space after the opening curly brace and 1 before the closing curly brace in a hash literal, so this:

{book_name: @book_name, author_name: @author_name, page_count: @page_count}

should be this:

{ book_name: @book_name, author_name: @author_name, page_count: @page_count }

No empty parameter list

In Ruby, if a method or a block takes no parameters, it is standard to not define an empty parameter list but simply leave it out completely.

So, this

def initialize()

should just be

def initialize

No empty argument list

In Ruby, if a message send has no arguments, it is standard to not write an empty argument list but simply leave it out completely.

So, this

books = Book.new()

should just be

books = Book.new

Vertical whitespace

There should be a blank line after every "logical" break. In particular, there should be a blank line after the attr_accessor:

attr_reader :book_name, :author_name, :page_count, :more, :book_array

def initialize

Also, 3 blank lines after the class body are a bit excessive. 1 blank line is standard according to most style guides. I can understand 2, but not 3. I would prefer 1.

There should be no blank line before the end of a block, whether that is an actual end keyword, a closing parenthesis, etc. For example here:

``lang-ruby book_list

end


This should just be

``lang-ruby
  book_list
end

It could also help readability if you break up the ask method:

puts 'Name of book'
@book_name = gets.chomp

puts 'Author of the book'
@author_name = gets.chomp

puts 'Page count of the book'
@page_count = gets.to_i

puts 'Do you want add more book'
@more = gets.chomp.capitalize

Code Formatting

If possible, you should set your editor or IDE to automatically format your code when you type, when you paste, and when you save, and set up your version control system to automatically format your commit when you push, as well as set up your CI system to reject code that is not correctly formatted. If not possible, you should seriously consider using a different editor or IDE, version control system, or CI system.

Here's the result of your code, when I simply paste it into my editor, without doing anything else (I am literally just copying the code from your question and pasting it into my editor, and my editor auto-formats it):

class Book
  attr_reader :book_name, :author_name, :page_count, :more, :book_array

  def initialize
    @book_name = ''
    @author_name = ''
    @page_count = 0
    @more = 'yes'
    @book_array = []
  end

  def answers_start
    until more_book?
      ask
      add_arr
    end

    book_list
  end

  private

  def ask
    puts 'Name of book'
    @book_name = gets.chomp
    puts 'Author of the book'
    @author_name = gets.chomp
    puts 'Page count of the book'
    @page_count = gets.to_i
    puts 'Do you want add more book'
    @more = gets.chomp.capitalize
  end

  def more_book?
    @more == 'No'
  end

  def book_list
    @book_array.each_with_index do |item, index|
      puts "Book#{index + 1} {Name: #{item[:book_name]}, Author: #{item[:author_name]}, Page count: #{item[:page_count]}}"
    end
  end

  def add_arr
    book_array << { book_name: @book_name, author_name: @author_name, page_count: @page_count }
  end
end

books = Book.new
books.answers_start

As you can see, simply copying your code into my editor, the editor corrected every single thing I wrote above except one.

Let me repeat that: I just spent 3 pages pointing out all the style inconsistencies and recommendations, and you could just have fixed all of that in a couple of milliseconds at the push of a button!

Frozen string literals

Immutable data structures and purely functional code are always preferred, unless mutability and side-effects are required for clarity or performance. In Ruby, strings are always mutable, but there is a magic comment you can add to your files (also available as a command-line option for the Ruby engine), which will automatically make all literal strings immutable:

# frozen_string_literal: true

It is generally preferred to add this comment to all your files.

Linting

You should run some sort of linter or static analyzer on your code. Rubocop is a popular one, but there are others.

Rubocop was able to detect all of the style violations I pointed out above (plus some more), and also was able to autocorrect all of them except one.

Let me repeat that: I have just spent two pages pointing out how to correct tons of stuff that you can actually correct within milliseconds at the push of a button. I have set up my editor such that it automatically runs Rubocop with auto-fix as soon as I hit "save".

In particular, running Rubocop on your code, it detects 32 offenses, of which it can automatically correct 31.

Here's what the result of the auto-fix looks like:

# frozen_string_literal: true

class Book
  attr_reader :book_name, :author_name, :page_count, :more, :book_array

  def initialize
    @book_name = ''
    @author_name = ''
    @page_count = 0
    @more = 'yes'
    @book_array = []
  end

  def answers_start
    until more_book?
      ask
      add_arr
    end

    book_list
  end

  private

  def ask
    puts 'Name of book'
    @book_name = gets.chomp
    puts 'Author of the book'
    @author_name = gets.chomp
    puts 'Page count of the book'
    @page_count = gets.to_i
    puts 'Do you want add more book'
    @more = gets.chomp.capitalize
  end

  def more_book?
    @more == 'No'
  end

  def book_list
    @book_array.each_with_index do |item, index|
      puts "Book#{index + 1} {Name: #{item[:book_name]}, Author: #{item[:author_name]}, Page count: #{item[:page_count]}}"
    end
  end

  def add_arr
    book_array << { book_name: @book_name, author_name: @author_name, page_count: @page_count }
  end
end

books = Book.new
books.answers_start

And here are the offenses that Rubocop could not automatically correct:

Inspecting 1 file
C

Offenses:

book.rb:3:1: C: Style/Documentation: Missing top-level documentation comment for class Book.
class Book
^^^^^^^^^^
book.rb:42:121: C: Layout/LineLength: Line is too long. [122/120]
      puts "Book#{index + 1} {Name: #{item[:book_name]}, Author: #{item[:author_name]}, Page count: #{item[:page_count]}}"
                                                                                                                        ^^

1 file inspected, 2 offenses detected

By the way, you might have noticed that I wrote above that Rubocop detected only 1 uncorrectable offense, yet after running it on the code, we are left with 2. That is because adding the space around the operator in index + 1 actually pushed the line over the maximum length.

Similar to Code Formatting, it is a good idea to set up your tools such that the linter is automatically run when you paste code, edit code, save code, commit code, or build your project, and that passing the linter is a criterium for your CI pipeline.

In my editor, I actually have multiple linters and static analyzers integrated so that they automatically always analyze my code, and also as much as possible automatically fix it while I am typing. This can sometimes be annoying (e.g. I get 75 notices for your original code, lots of which are duplicates because several different tools report the same problem), but it is in general tremendously helpful. It can be overwhelming when you open a large piece of code for the first time and you get dozens or hundreds of notices, but if you start a new project, then you can write your code in a way that you never get a notice, and your code will usually be better for it.

Inconsistent use of attribute methods and instance variables

You are almost always accessing instance variables directly, e.g. here

@more == 'No'

which should be

more == 'No'

In fact, you only use the attribute once and only for one of your attributes, namely here:

book_array << { book_name: @book_name, author_name: @author_name, page_count: @page_count }

Here you are using the attr_reader for book_array while on the very same line not using the attr_readers for book_name, author_name, and page_count. In fact, you are never using the attr_readers for book_name, author_name, page_count, or more.

This is inconsistent. Choose one or the other.

I personally prefer to always use the attribute methods, because methods are more flexible: they can be overridden in subclasses or their implementation can be changed, without having to change any of the client code.

So, you should either change the second example to

@book_array << { book_name: @book_name, author_name: @author_name, page_count: @page_count }

or (my preference) to

book_array << { book_name: book_name, author_name: author_name, page_count: page_count }

and the same in a couple of other places.

Exposing mutable state

Your book_array is only an attr_reader, which means that another object is not allowed to assign to it. However, that does not actually stop anybody from messing up your state: since book_array exposes an Array, and Arrays are mutable, someone could just change the array itself.

For example, I could do this:

books = Book.new
books.book_array << nil
books.answers_start

And then the program will blow up with a NoMethodError exception when book_list tries to execute nil[:book_name].

You should never expose mutable internal state that way. You should at least copy and freeze the object like this, using a custom attribute reader instead of the auto-generated one:

class Book
  private

  attr_reader :book_name, :author_name, :page_count, :more

  # …

  public

  def book_array
    @book_array.dup.freeze
  end
end

However, that is actually still not safe, because the same applies to the hashes inside of that array: Those are also mutable and they also can be changed from the outside, so they should be frozen as soon as they are inserted into the array. And, you may have guessed it already: it applies to the strings inside the hash inside the array, too.

def ask
  puts 'Name of book'
  @book_name = gets.chomp.freeze

  puts 'Author of the book'
  @author_name = gets.chomp.freeze

  puts 'Page count of the book'
  @page_count = gets.to_i

  puts 'Do you want add more book'
  @more = gets.chomp.capitalize.freeze
end

def add_arr
  book_array << { book_name: @book_name, author_name: @author_name, page_count: @page_count }.freeze
end

Note, however, that when you do this, then your add_arr method actually no longer works as intended: first off, it will raise an exception, because book_array now returns a frozen array that does not allow you to add a book anymore, and also, even if it were not frozen, it returns a new duplicate of the array every time, so even if you could add a book to it, that modification would immediately be lost.

So, there are two ways around this: one would be to directly use the instance variable in add_arr:

def add_arr
  @book_array << { book_name: @book_name, author_name: @author_name, page_count: @page_count }.freeze
end

And the other would be to add a separate private attribute reader that gives access to the original instance variable in addition to the public attribute reader that only exposes a frozen duplicate.

However, see the next section.

Access Restrictions

As far as I can see, none of your attr_readers are intended to be used by other objects. In fact, most of them shouldn't be used by other objects! They are private internal state of the book object. For example, there is no reason for anyone except the book itself to access more. Therefore, all of them should not be part of the public API, they should be private:

private

attr_reader :book_name, :author_name, :page_count, :more, :book_array

Unnecessary assignment

@book_name, @author_name, and @page_count get overwritten by ask as soon as the program starts. There is no reason to assign them in the initializer, if they immediately get overwritten anyway, so the initializer should just be

def initialize
  @more = 'yes'
  @books = []
end

Hungarian Notation

You are sometimes using something that resembles Hungarian Notation but is not quite it. The original Hungarian notation, invented by Charles Simyoni at Xerox PARC, is about encoding semantic information in an identifier name (one of the examples by Simyoni is the use of the prefix us to mark an "unsafe string", i.e. a string that was supplied as user input and should this be treated as untrusted). However, you are mostly using it to encode the class name of the object, e.g. in book_array and add_arr. (Sidenote, speaking of consistency: why is one named arr and the other array?)

This is pretty much unnecessary. It is also not exactly true: neither the @book_array instance variable nor the add_arr method actually require an Array. Both of them would also work with a variety of other types, in fact, the only thing they require are the messages << and each_with_index.

If you want to express that something is a collection, this is usually done by simply naming it with a plural, for example, @book_array could simply be named @books.

Naming

There are a some names that are somewhat confusing, misleading, or could be expressed better. I already mentioned @book_array which should just be @books.

Not only does add_arr use Hungarian Notation without a real need to, but it also isn't even correct: add_arr doesn't add an array, it adds a book! So, it should be named add_book, but since this is a library system, it is probably obvious that we are adding books, so it could just be called add. Well, except, actually, it doesn't even add a Book, it adds a Hash, but more on that later …

book_list is another misleading name. I would expect a method named book_list to be an attr_reader that returns a book list, i.e. a list of books. Instead, it is a command that lists the books, so at the very least it should be named with a verb to make it clear that it performs a command not a query. So, at the very least, it should be named list_books.

The two block parameters in the call to @books.each_with_index could use some love, too. @books is supposed to be a list of books, so when you iterate over it, what do you get? You get a book, not an item. So, this parameter should be named item. On the other hand, index could just be named i, which is a well-known name for an index. However, Reek complains about that, so we'll call it idx. (Side note: if you disagree with a default setting in a code formatter or a linter or a static analyzer, don't be afraid to change it!)

more_book? is also confusing. It asks about more books, but when the answer is true, then that actually means that there are no more books. So, the method is actually the wrong way round.

But the most confusing name of all is the class: Book. Because it is actually not a book. In fact, when you instantiate it, you assign it to a variable named books, which clearly indicates that it is not a book.

But actually books is not correct either, because Book is actually both a list of books and an application that asks questions, prints stuff, etc. In fact, Book is pretty much everything except a book! A book is actually a Hash in your design, namely the one constructed in your add_arr method.

Error handling and input validation

There is zero error handling or input validation in your code. If someone enters "eight hundred" for the page count, the program raises an exception. If someone enters "Stop" for the question about more books, the program continues.

You should validate inputs by the user, and handle wrong inputs appropriately.

Mixing I/O and computation

In the list_books method, we are mixing I/O and computation by both building a string representation of a library and printing it out. In general, I/O and computation should be segregated and I/O should be relegated to the outer layers of the system.

So, the list_books method should simply return a string representation (and should probably be called to_s), and then the application can print this string … or do something else with it. That is another problem with this design: you can only print a list of books. You cannot display it on a website, for example.

It is not the responsibility of a book to print something, it is not the responsibility of a book to know about other books and libraries, and it is not the responsibility of a book to ask the user questions.

Excessive instance variables

You have a lot of instance variables. But apart from @books, none of them are actually used to hold state of the object. They are only used to pass information back and forth between the various methods. So, we should probably make them parameters, arguments, and return values instead. Something like this:

class Book
  private

  attr_reader :books

  def initialize
    @books = []
  end

  public

  def answers_start
    more = 'Yes'
    while more == 'Yes'
      book, more = ask
      add_book(book)
    end

    list_books
  end

  private

  def ask
    puts 'Name of book'
    book_name = gets.chomp.freeze

    puts 'Author of the book'
    author_name = gets.chomp.freeze

    puts 'Page count of the book'
    page_count = gets.to_i

    puts 'Do you want add more book'
    more = gets.chomp.capitalize.freeze

    [{ book_name: book_name, author_name: author_name, page_count: page_count }.freeze, more]
  end

  def list_books
    books.each_with_index do |book, idx|
      puts "Book#{idx + 1} {Name: #{book[:book_name]}, Author: #{book[:author_name]}, Page count: #{book[:page_count]}}"
    end
  end

  def add_book(book)
    books << book
  end
end

books = Book.new
books.answers_start

Overall design

The overall design is very weird, and really does not make much sense. There is no coherence in the class, it does many different things, it doesn't really have any state, and it is being used as a singleton.

It looks like a bunch of procedural code with a class … end wrapped around it for no reason. In fact, the code would be much better without the class.

In object-orientation, objects collaborate with each other by sending messages, but here, there is really only one object, books, which does everything. (Of course, technically speaking, all the strings and numbers and hashes and arrays are also objects, but they are not Domain Objects.)

Personally, I can see at least three different kinds of objects here: we have books, we have lists of books (libraries), and we have library management applications.

Maybe something like this:

# frozen_string_literal: true

class Book
  include Enumerable

  attr_reader :title, :author, :page_count

  private

  attr_writer :title, :author, :page_count

  def initialize(title, author, page_count)
    self.title = title.dup.freeze
    self.author = author.dup.freeze
    self.page_count = page_count

    freeze
  end

  public

  def <=>(other)
    return nil unless other.is_a?(Book)
    [title, author, page_count] <=> [other.title, other.author, other.page_count]
  end

  def to_h
    { title: title, author: author, page_count: page_count }
  end

  def to_s
    "{ Title: #{title}, Author: #{author}, Page count: #{page_count} }"
  end

  freeze
end

class Library
  include Enumerable

  private

  attr_writer :books

  def initialize(*books)
    self.books = books

    freeze
  end

  public

  def <<(...)
    @books.<<(...)
    self
  end

  def add(*books)
    books.each(&method(:<<))
    nil
  end

  def books = @books.dup.freeze
  alias_method :to_a, :books

  def each(...) = books.each(...)

  def to_s =
    books.each.with_index(1).map {|book, idx| "Book#{idx} #{book}" }.join("\n")

  freeze
end

class App
  private

  attr_accessor :library

  def initialize = self.library = Library.new

  public

  def run
    done = false

    until done
      book, done = ask_user

      library << book
    end

    print_library
  end

  private

  def ask_user
    title = user_input('Please, enter the title of the book:', :string)
    author = user_input('Please, enter the author of the book:', :string)
    page_count = user_input('Please, enter the number of pages of the book:', :integer)

    done = !user_input('Do you want to add another book?', :boolean)

    [Book.new(title, author, page_count), done]
  end

  def user_input(question, type)
    print "#{question} "
    input = gets.chomp

    case type
    when :string
      input
    when :integer
      raise ArgumentError.new("Input should be an integer instead of `#{input}`") unless input =~ /^\d+$/
      input.to_i
    when :boolean
      input = input.downcase
      raise ArgumentError.new("Input should be an boolean instead of `#{input}`") unless input =~ /^[yntf]/
      input =~ /[yt]/
    end
  end

  def print_library = puts(library)
end

App.new.run

Of course, there is still a lot of room for improvement here. For example, I can imagine an InputValidator, StringReader, IntegerReader, BooleanReader, and probably a BookReader. There could be much better input validation. There is a lot of potential for the Replace Conditional with Polymorphism Refactoring – in general, it should always be possible to write an OO program without any conditionals at all, except for message dispatch.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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