Skip to content
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

Chitter Challenge Pull Request #2196

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

aubreysalmins
Copy link

I hope I've done this pull request right! :/

peep.title = params[:title]
peep.content = params[:content]
peep.time_stamp = Time.new
peep.user_id = 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to tidy this up a bit more - by tidying, I mean moving some of the 'business logic' out of the app.rb - you could write something like peep = Peep.new(params[:title], params[:content], etc., passing in these parameters as arguments to a new Peep object, and storing the variables that way instead.

The benefit of doing this is that you push some of the responsibility to the Peep class instead of the app.rb. This will adhere more closely to 'Single Responsibility Principle', which guides us to try to give each class one overarching responsibility.

In the context of a full stack web app, the pattern we follow is Model View Controller, or MVC. In this case, app.rb is the Controller - it's responsible for responding to URL requests / button actions etc. and deciding which code to run - the Models are responsible for structuring our objects like Peeps and Users, and communicating with the database through the Repository classes. And the Views are responsible for what's visible to the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants