Refactoring the code #4
Conversation
There was a problem hiding this comment.
STATUS: CHANGES REQUESTED ♻️
Hi @barackm, You did a great job so far 👏🏼 👏🏼
- Well done on separating the logic into separate files
However, here are some points that you need to work on to get your project approved.
Required changes :
Check the inline comments.
Optional changes :
Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
people/main.rb
Outdated
| @@ -0,0 +1,58 @@ | |||
| # rubocop:disable Metrics\CyclomaticComplexity, Metrics/MethodLength | |||
There was a problem hiding this comment.
- Kindly remove this comment to enable the linters and modify your code to fix the errors. Linters help you write clean code.
- Kindly do the same in all the other files where linters are disabled.
people/main.rb
Outdated
| def create_person | ||
| print 'Do you want to create a student (1) | ||
| or a teacher (2)? [Input the number]: ' | ||
| option = gets.chomp | ||
|
|
||
| case option | ||
| when '1' | ||
| print 'Age: ' | ||
| age = gets.chomp | ||
|
|
||
| print 'Name: ' | ||
| name = gets.chomp | ||
|
|
||
| print 'Has parent permission? [y/n]: ' | ||
| parent_permission = gets.chomp == 'y' | ||
|
|
||
| student = Student.new(age, name, parent_permission) | ||
| @people << student | ||
| when '2' | ||
| print 'Age: ' | ||
| age = gets.chomp | ||
|
|
||
| print 'Name: ' | ||
| name = gets.chomp | ||
|
|
||
| print 'Specialization: ' | ||
| specialization = gets.chomp | ||
|
|
There was a problem hiding this comment.
- I think this method needs refactoring. Currently, the method has many responsibilities which go against code best practices especially the SOLID principles. You can check this link to better understand the concept. Kindly refactor this method, by distributing its code into small methods each with a single responsibility.
rentals/main.rb
Outdated
| def create_rental(books, people) | ||
| puts 'Select a book from the following list by number' | ||
| books.each_with_index { |book, index| puts "#{index}) Title: #{book.title}, Author: #{book.author}" } | ||
|
|
||
| book_index = gets.chomp.to_i | ||
|
|
||
| puts 'Select a person from the following list by number (not id)' | ||
| people.each_with_index do |person, index| | ||
| puts "#{index}) [#{person.class}] Name: #{person.name}, ID: #{person.id}, Age: #{person.age}" | ||
| end | ||
|
|
||
| person_index = gets.chomp.to_i | ||
|
|
||
| print 'Date: ' | ||
| date = gets.chomp | ||
|
|
||
| rental = Rental.new(date, books[book_index], people[person_index]) | ||
|
|
||
| @rentals << rental | ||
| puts 'Rental created successfully' | ||
| end |
There was a problem hiding this comment.
- Kindly refactor this function. As I mentioned in the previous requested change it goes against best code practices DRY and SOLID principles. For example, you can make use of the list_books and list_people methods you already created instead of repeating the same blocks inside this method.
In this PR I have implemented the following