-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/associations #3
Conversation
…the instance variable label on the constructor, and getters and setters for that variable using attr_accesor. In addition, Create the has-many/belongs-to relationship between Classroom and Student"
…structor the instance variables title and author, and add getters and setters instances to title, author and rentals
…tance variable date on the cosntructor and creates getters and setters for date with attr_accessor. In addition add a relationship many-to-many betwin person and book
… 'has-many' relationship with the Rental class. Adds a method for adding new rentals to a person's list of rentals. And Updates access methods and attributes as per requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job so far, indeed you have spent time on this project!
There are some issues that you still need to work on to go to the next project but you are almost there!
Highlights
- Readme file professional ✔️
- Linters passed ✔️
- Gitflow ✔️
Required Changes ♻️
Check the comments under the review.
- I realized that you did not create the classroom method which should be responsible for setting the classroom for a student and also add it to the classrooms' students. Remember the relationship too.
What you have now
Optional suggestions
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, remember to tag me in your question so I can receive the notification.
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.
book.rb
Outdated
# this class represents the books | ||
class Book | ||
attr_accessor :title, :author | ||
attr_reader :rentals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The rentals should be set for getter and setter since we will also be adding and reading it from the UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Olamarx. Done!
book.rb
Outdated
@rentals = [] | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- There should be a method for adding rentals here, just as you have it in person class but would have parameters for person and date so that it would serve that purpose but peculiar rentals here. Remember that in the method, the book itself should be sent as a parameter to create the new rental achieving the 'has-many' association
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Olamarx. Done!
rental.rb
Outdated
# this class represents the rental | ||
class Rental | ||
attr_accessor :date | ||
attr_reader :book, :person |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Getters and Setters for book and person but you are only setting the Getter, it is necessary for the two to be writeable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Olamarx. Done!
… the students array on the classroom object only if the student is not yet on the array.
…rameters for person and date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ClaudiaRojasSoto 👏 👏,
Your project is complete! There is nothing else to say other than... it's time to merge it
Congratulations! 🎉
Highlights
Set up associations between classes and objects. ✔️
Optional suggestions
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
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.
Thank you @OyePriscilla!! |
Description
This pull request aims to integrate the final classes and their associations for our school library project. The changes align with the project requirements and Ruby best practices.
General Requirements
Implemented Features
Classroom Class
@label
instance variable with initialization in the constructor.@label
usingattr_accessor
.Student
class.Book Class
@title
and@author
instance variables with initialization in the constructor.attr_accessor
.Rental Class
@date
instance variable with initialization in the constructor.@date
usingattr_accessor
.Person-Book-Rental Associations
Book
andRental
.Rental
andBook
.Person
andRental
.Rental
andPerson
.Rental
to setBook
andPerson
.Please, review this project feature and give me feedback to improve it.
Greetings,
@ClaudiaRojasSoto