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.
