search

ltfschoen / littlehumans

Medical Portal App for Midwives developed with Ruby on Rails, Google Calendar, OmniAuth2, and PostgreSQL
http://littlehumans.herokuapp.com/
0 stars 0 forks source link

UserController is too fat #10

Open mikel opened 9 years ago

mikel commented 9 years ago

In your UsersController, you have too much logic in there. You want controller actions to be a few lines at most. Try moving all the twitter and google stuff out into the models or to other objects like service objects. A good read for this is http://blog.codeclimate.com/blog/2012/10/17/7-ways-to-decompose-fat-activerecord-models/. Warning though, it's really easy to go overboard with this stuff

ltfschoen commented 9 years ago

Moved Twitter Feed Client Request to a Module in Library/Assets Folder. Changes pushed with Branch named "feature/4_socialusers".

mikel commented 9 years ago

OK, well done, this is good. There's some small things I would change that I can give feedback on later. But a couple of things to note:

1) the app/helpers folder is for helper modules, these are used by views to remove as much logic as possible from the views. This would be better in lib/social_oauth.rb and wouldn't have to be wrapped in the UserHelper namespace then. You'd then call it from the User model with just SocialOauth. 2) the lib/assets folder is part of the asset pipeline, so you don't want to put Ruby in here. Take a read of http://guides.rubyonrails.org/asset_pipeline.html for more data on this 3) I'd have put the google, twitter and facebook items into lib/social_authenticators or something like that

Try making these changes and let me know.

mikel commented 9 years ago

If you can do this one as a proper pull request, that would be great :)

mikel commented 9 years ago

Also, in the OAuth module, I would split out those two chunky if statements starting on lines 9 and 23 into two methods making the client_check_provider_uid_email method much shorter and more concise.