# Library Program in Ruby with Class

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

until more_book?
end

book_list

end

private

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

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

end

books = Book.new()


# 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

until more_book?
end

book_list
end

private

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

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

books = Book.new


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

until more_book?
end

book_list
end

private

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

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

books = Book.new


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


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

# …

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

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


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

def initialize
@books = []
end

public

more = 'Yes'
while more == 'Yes'
end

list_books
end

private

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

books << book
end
end

books = Book.new


# 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

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

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

library << book
end

print_library
end

private

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.