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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Features/user controller #42

Merged
merged 9 commits into from
Dec 19, 2015
Merged

Features/user controller #42

merged 9 commits into from
Dec 19, 2015

Conversation

npauzenga
Copy link
Owner

Happy weekend @enriikke!

I finished a pass on the users_controller, complete with interactors and specs all around. I'm going to add a few notes to things that might be worth checking out but please have a look and let me know what can be improved!

On futures PRs I'll submit all of the tests before the functionality. I just wanted to go through one (using TDD) to figure out what the tests were going to look like in the end 馃榿

else
render json: result.error, status: :not_found
end
end
Copy link
Owner Author

Choose a reason for hiding this comment

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

@enriikke on #show and #destroy I've got else branches that handle errors. I don't often see this in other controllers I've looked at. Should I simplify these to the basic assignment (@user = User.find(:id) or similar) and forget about the elses? Seems like it's always better to have a safety net?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to prefer to maintain consistency and to keep using interactors throughout the app, even in simple cases like this. That's just my preference thought 馃槃. You can argue that abstracting something so simple into an interactor could make it harder to read.

It also depends on what you are doing. For some apps, show could required a more complex flow that you might want to check what the result was. Rails automatically handles not_found for ActiveRecord and Postgres, but if you decide to use something else then you have to catch those errors yourself. The interactor patter helps with that.

I think it really boils down to a matter of preference. Rails has conventions for things and some people like just following the same conventions. It makes it easier to read and understand an app if you know where to look. 馃槂

@enriikke
Copy link
Collaborator

Looking good @npauzenga!! :shipit: 馃殌 馃悡

npauzenga added a commit that referenced this pull request Dec 19, 2015
@npauzenga npauzenga merged commit 1f1464e into staging Dec 19, 2015
@npauzenga npauzenga deleted the features/user-controller branch December 19, 2015 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants