Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,12 @@ def destroy
end

def following
@title = "Following"
@user = User.find(params[:id])
@users = @user.followed_users.paginate(page: params[:page])
@subtitle = "You Are Following #{@user.followed_users.size} Bloggers"
@presenter = Users::FollowedUsersPresenter.new(params[:id], params[:page])
render 'show_follow'
end

def followers
@title = "Followers"
@user = User.find(params[:id])
@users = @user.followers.paginate(page: params[:page])
@subtitle = "Your Got #{@user.followers.size} Followers"
@presenter = Users::FollowersPresenter.new(params[:id], params[:page])
render 'show_follow'
end

Expand Down
18 changes: 18 additions & 0 deletions app/presenters/users/follow_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
module Users
class FollowPresenter
include Draper::ViewHelpers

def initialize(user_id, page)
@user_id = user_id
@page = page
end

def user
@user ||= User.find(@user_id)
end

def cache_key
[user, title]
end
end
end
15 changes: 15 additions & 0 deletions app/presenters/users/followed_users_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module Users
class FollowedUsersPresenter < FollowPresenter
def users
@users ||= user.followed_users.paginate(page: @page)
end

def title
"Following"
end

def subtitle
@subtitle ||= "You Are Following #{h.pluralize(user.followed_users.size, "Blogger")}"
end
end
end
16 changes: 16 additions & 0 deletions app/presenters/users/followers_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
module Users
class FollowersPresenter < FollowPresenter
def users
@users ||= user.followers.paginate(page: @page)
end

def title
"Followers"
end

def subtitle
@subtitle ||= "Your Got #{h.pluralize(user.followers.size, "Followers")}"
end
end
end

24 changes: 12 additions & 12 deletions app/views/users/show_follow.html.erb
Original file line number Diff line number Diff line change
@@ -1,32 +1,32 @@
<% provide(:title, @title) %>
<% provide(:title, @presenter.title) %>

<% cache [@user, @title] do %>
<% cache @presenter.cache_key do %>
<div class="row">
<aside class="span4">
<section>
<%= gravatar_for @user %>
<h1><%= @user.name %></h1>
<span><%= link_to "view my profile", @user %></span>
<span><b>Microposts:</b> <%= @user.microposts.count %></span>
<%= gravatar_for @presenter.user %>
<h1><%= @presenter.user.name %></h1>

Choose a reason for hiding this comment

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

Does this here violate the "Law" of Demeter? The View here needs to know about the User model. It might be better to have a method named "user_name" in the presenter. This way the presenter is the only one that needs to know about the User model. The same could be said for "@presenter.user.microposts.count", although perhaps the User model (or some other object) should have a "microposts_count" method so that the presenter doesn't know about the Microposts model either.

This way, the view is no longer coupled to any other objects but the presenter. It's much easier to test, because you can easily stub out these methods, and it would be much easier to change implementation without breaking things.

Just a thought. :)

By the way, I really enjoyed your talk from RailsConf on this subject. It has given me lots to consider and try for myself. Great work!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @dsibiski, Thanks! I don't really see the value of adding a user_name method on the presenter, as it would be just a pass through. However, if there was some sort of modification to the @presenter.user.name, such as needing to to modify the value if it were too long, then that would be a fine example to justify a new method.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Systho The reason I separate the concept of presenters and decorators is that I'm using the concept of a presenter as very specifically for a single view or controller action, and the decorator as something more closely related to the model that could be used on many views and controller actions. Thus, I might start with the code in the presenter and then if that logic was needed elsewhere in the program, I'd be tempted to break out the method into one on the decorator.

Choose a reason for hiding this comment

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

@justin808 You're right when you say that the method would just be a pass through, but unfortunately, without it, you are introducing some unnecessary coupling to 3rd party objects. In my opinion, that is one of the purposes of having a Presenter object. To eliminate the coupling between the View layer and the Model/data layer.

There is a portion of an article on "The Pragmatic Bookshelf" that addresses this issue exactly...under the subhead "Law of Demeter" in the article "Tell, Don't Ask" it says:

The disadvantage, of course, is that you end up writing many small wrapper methods that do very little but delegate container traversal and such. The cost tradeoff is between that inefficiency and higher class coupling.

The higher the degree of coupling between classes, the higher the odds that any change you make will break something somewhere else. This tends to create fragile, brittle code.

Depending on your application, the development and maintenance costs of high class coupling may easily swamp run-time inefficiencies in most cases.

Reference: https://pragprog.com/articles/tell-dont-ask

There is also a really great talk by Ben Orenstein that touches on these points and why they're beneficial. Check it out: https://www.youtube.com/watch?v=C0H-LyZy9Ko

You also mentioned in your previous comment:

However, if there was some sort of modification to the @presenter.user.name, such as needing to to modify the value if it were too long, then that would be a fine example to justify a new method.

I think the problem here is that when you change the model, you have to remember that you need to change things not only in the presenter, but also in the view (and perhaps in other classes - real apps are always more complex). If the presenter was the only one coupled to the Model, then this would be far less of an issue.

Of course, one could go crazy with this stuff and it might not be practical to follow this religiously for every circumstance, however, it has surely helped me to keep coupling low and save me tons of headaches especially when more features and complexity are introduced into the system.

Choose a reason for hiding this comment

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

You can delegate specific methods to the user object. E.g. user_name would delegate the name method to the user. The key thing would be decouple the view with knowing how the what to go through to get the user name. Allows the presenter could override this and provide implementation of user_name if necessary.

<span><%= link_to "view my profile", @presenter.user %></span>
<span><b>Microposts:</b> <%= @presenter.user.microposts.count %></span>
</section>
<section>
<%= render 'shared/stats' %>
<% if @users.any? %>
<% if @presenter.users.any? %>

Choose a reason for hiding this comment

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

tell, don't ask. you can get rid of both of these predicates on .any? by moving the logic into a helper or partial.

if the only point in these predicates are to prevent @users.each and @presenter.users.each from chunking on you, then you can do a @presenter.users.to_a.... and @users.to_a...

Then they wont blow up if there isnt anything in them as they become empty arrays!

<div class="user_avatars">
<% @users.each do |user| %>
<% @presenter.users.each do |user| %>
<%= link_to gravatar_for(user, size: 30), user %>
<% end %>
</div>
<% end %>
</section>
</aside>
<div class="span8">
<h3><%= @subtitle %></h3>
<% if @users.any? %>
<h3><%= @presenter.subtitle %></h3>
<% if @presenter.users.any? %>

Choose a reason for hiding this comment

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

so many nil checks.... :) your system is becoming quickly infested!

Choose a reason for hiding this comment

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

nullObjectPattern or confient casting or tell don't ask - there are 3 mechanisms for ya to combat this!

<ul class="users">
<%= render @users %>
<%= render @presenter.users %>
</ul>
<%= will_paginate %>
<%= will_paginate @presenter.users %>
<% end %>
</div>
</div>
Expand Down
4 changes: 4 additions & 0 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,9 @@ class Application < Rails::Application
I18n.enforce_available_locales = true

config.assets.precompile += %w(*.png *.jpg *.jpeg *.gif)

config.autoload_paths += %W(
#{config.root}/app/presenters
)
end
end