Topic: Refactoring My Application

Well, my application is pretty much ready for production. However, as I have been coding it for a few months, it has began to be bloated. I'm considering just creating a new app and copying the code I need over to the new one. That would take a while and I'm not sure I'm ready for that sort of jump. I'd much rather just refactor and learn to do things better. I'm gonna show you some examples of the source and see what you think I should do to begin with?

Let's start with the Album Controller

 
require "amazon/search"
include Amazon::Search
class AlbumsController < ApplicationController
before_filter :login_required, :except => [:index,:show]
auto_complete_for :artist, :title
def index
@albums = Album.find(:all, :order => 'created_at DESC', :include => @artist)
@artists = Artist.find(:all)
end

def search
end

def list
  @albums = Album.find(:all, :order => 'title')
end

def show
@albums = Album.find(:first, :conditions => ['title_copy = ?',params[:albumtitle]])
end

def search
  render :layout => false
end

def artists_for_lookup
  @artists = Artist.find(:all)
  @headers['content-type'] = 'text/javascript'
  render :layout => false
end

def new
@album = Album.new
@artist = Artist.new
end

def create
@album = Album.new(params[:album])
@album.title = :product.product_name
@album.added_by = session['user'].login
if @album.save
  redirect_to :controller => 'main', :action => 'index'
else
  render :controller => 'albums', :action => 'new'
  @flash = "Something Went Wrong"
end
end

def edit
@album = Album.find(params[:id])
end

def add_artist
  render :partial => "artists/form"
end

def update
  @album = Album.find(params[:id])
  if @album.update_attributes(params[:album])
    flash[:notice] = 'Album was successfully updated.'
    redirect_to album_url(:albumtitle => @album.title.downcase)
  else
    render :action => 'edit'
  end
end

def destroy
  @album = Album.find(params[:id])
  @album.destroy
  redirect_to :action => 'index'
end
def say_when
  render_text "<p>The time is <b>" + DateTime.now.to_s + "</b></p>"
end

end


And now the Album Index. I feel like there is just too much logic in the view. The only problem is that I don't know what I should do about it?

<%= javascript_include_tag "prototype" %>

<%if session['user']!=nil%>
<h2><u><%= link_to "New Album", :action => 'new'%></u></h2>
<%end%>
<hr />

<%@count = 0%>
<%for album in @albums%>
<%if @count <= 4%>
<%@count+=1%>
<%if @count == 1%>
<div class="post">
<div class = "floatRight">
<img src=<%=album.art%> width=250 height=250 align="left"/>
</div>
<h2><%=link_to album.title, album_url(:albumtitle => album.sanitized_title)%></h2>
<h4><%=album.artist.title%></h4>
<% unless album.review.nil? -%>
<%=truncate(album.review, 700)%>
<%end%>
<%= link_to '[read more]',album_url(:albumtitle => album.sanitized_title)%>
<%= link_to "buy via AmpCamp", "http://www.ampcamp.com/basic_search_res … p;select=1"%>
<%if session['user'] != nil %>
<%=link_to "edit", :controller => 'albums', :action => 'edit', :id => album.id%>
<%= link_to "buy via AmpCamp", "http://www.ampcamp.com/basic_search_res … p;select=1"%>
<%end%>
<%else%>
<hr />
<div class = "floatRight">
<img src=<%=album.art%> width=150 height=150 align="left"/>
</div>
<h2><%=link_to album.title, album_url(:albumtitle => album.sanitized_title)%></h2>
<h4><%=album.artist.title%></h4>
<% unless album.review.nil? -%>
<%=truncate(album.review, 400)%>
<%end%>
<%= link_to '[read more]',album_url(:albumtitle => album.sanitized_title)%>
<%= link_to "buy via AmpCamp", "http://www.ampcamp.com/basic_search_res … p;select=1"%>
<%if session['user'] != nil %>
<%=link_to "edit", :controller => 'albums', :action => 'edit', :id => album.id%>
<%= link_to "buy via AmpCamp", "http://www.ampcamp.com/basic_search_res … p;select=1"%>
<%end%>
<%end%>
<%end%>
<%end%>
</div>


And Finally, A Simple Blog COntroller

class BlogController < ApplicationController
  before_filter :login_required, :except => [:show, :index]
  def index
    @posts = Post.find(:all, :order => 'created_at DESC')
  end
  def new
    @post = Post.new
  end
  def create
    @post = Post.new(params[:post])
    @post.user_id = session['user'].id
  if @post.save
    redirect_to :action => "index"
  else
  render :action => 'new'
end
end
  def edit
    @post = Post.find(params[:id])
    if @post.user_id != session['user'].id
      redirect_to :action => 'index'
    end
  end
def show
  @post = Post.find(:first, :conditions => ['title = ?',params[:posttitle]])
end
def update
  @post = Post.find(params[:id])
  if @post.update_attributes(params[:post])
    redirect_to :action => 'show', :id => @post
  else
    render :action => 'edit'
  end
end
end

And A Blog View
This is our community blog. If you are a contributor, feel free to post whatever you want here.<br>
<%if session['user']!=nil%>
<h2><u><%= link_to "New Post", :action => 'new'%></u></h2>
<%end%>
<hr />
<%for post in @posts%>
<%unless session['user'] == nil%>
<%if post.user_id == session['user'].id%>
<div class = "post">
<div class = "title"><%=post.title%></div>
<p><%=post.created_at%>    <br />
<%= post.body%></p><br><u><%= link_to "edit", :action => "edit", :id => post.id %></u>
    by <%=post.user.login%>
    <br>
    </div>
<%else%>
<div class = "post">
<div class = "title"><%=post.title%></div><br />
<p><%=post.created_at%><br />
<p>    <%= post.body%></p><br>
    by <%=post.user.login%>
    <br>
    </div>
<%end%>
<%else%>
<div class = "post">
<div class = "title"><%=post.title%></div><br />
<p><%=post.created_at%><br />
<p>    <%= post.body%></p><br>
    by <%=post.user.login%>
    <br>
    </div>
<%end%>
<%end%>

This is just a preview of the main things on the site. I feel like so many things could look prettier and be simpler. I really want to refactor this and make use of helpers, partials, etc. Thanks.

Re: Refactoring My Application

Glad to see an interest in refactoring. Here's a list of bad smells and refactorings you can do to solve the problem.

Controllers

duplication
  - move to model
  - extract method
  - before filters
  - move to superclass
business logic (validations/complicated finds)
  - move to model
multiple scopes
  - move to controller/extract controller
long method (too many lines)
  - move to model
  - extract method
view logic
  - move to template/helper/partial

Views

duplication
  - move to partial
  - move to helper
  - move to model
  - move to controller
business logic
  - move to model
setting variable
  - move to controller
  - move to model
complicated view logic
  - move to helper

Models

duplication
  - extract to method
  - move to superclass or referenced class
view logic
  - move to template/helper/partial
long method
  - extract method

This list is far from complete but hopefully will give you an idea on how to refactor different problems.

The Albums controller looks pretty good. Most of the methods are small and don't contain business logic. Just a few points:

1. Why is "search" defined twice? You can remove the first definition.

2. There are two actions which center around artists more than albums (multiple scopes). I recommend moving this into an Artists controller.

3. You can shorten up the "show" action's find call to this (also, use singular variable name since it's only one album):
 

@album = Album.find_by_title_copy(params[:albumtitle])

4. The say_when method has view code. I think this would be better in a partial.

5. The "require" statement at the top should go in environment.rb

6. I don't think the "include" statement at the top does anything, you can remove it.

7. The second line of the "create" action is kind of strange, why are you calling a method on a symbol here?

8. In the "create" action I recommend using "flash[:notice]" or "flash[:error]" instead of the @flash instance variable.

9. The "index" action Album.find call has ":include => @artist". Perhaps you want ":include => :artists"?

I'll post again about refactoring the view.

Railscasts - Free Ruby on Rails Screencasts

Re: Refactoring My Application

mmm, Ryan. This looks fantastic. Can I buy you a beer or something? I really appreciate the time you take to help us all out.


edit:

9. The "index" action Album.find call has ":include => @artist". Perhaps you want ":include => :artists"?

I'm not even sure what the :include thing does?

Last edited by ldenman (2007-02-08 18:23:27)

Re: Refactoring My Application

The :include thing will fetch the album's artist the same time you fetch the albums - all in one big query. This is called eager loading. It probably won't change the behavior of the application, but it may improver performance if you need to display each album's artist. If you aren't concerned about performance you can just remove it:

@albums = Album.find(:all, :order => 'created_at DESC')

Railscasts - Free Ruby on Rails Screencasts

Re: Refactoring My Application

Onto the view. The user check at the beginning can be refactored into a helper method:

# in application_helper.rb
def logged_in?
  !session['user'].nil?
end

<% if logged_in? %>
  <h2><u><%= link_to "New Album", :action => 'new'%></u></h2>
<% end %>

No doubt you will need to perform this check frequently. This refactoring improves readability and moves this check into one location in case you need to change how it is performed.

In the next part of the app you are setting a variable. This is a smell mentioned in my previous post.

<%@count = 0%>
<%for album in @albums%>
  <%if @count <= 4%>
    <%@count+=1%>
    <%if @count == 1%>
      ...
    <%else%>
      ...
    <%end%>
  <%end%>
<%end%>
</div>

It is difficult to extract this variable because it changes within the loop. But, what are you using it for? It accomplishes two things:

1. determines if it's the first album
2. only displays the first four albums

You can move the 2nd one (the limit) into the controller's find call like this:

@albums = Album.find(:all, :order => 'created_at DESC', :limit => 4)

Now you just need to determine if it's the first album in the list. This can be done with the "first" method in the Array object. Like this:

<% for album in @albums %>
  <% if album == @albums.first %>
    ...
  <% else %>
    ...
  <% end %>
<% end %>

Much cleaner.

Another problem is the duplication in this "if" condition. The two statements are nearly identical. The only difference I see is the top line on each one:

<% for album in @albums %>
  <% if album == @albums.first %>
    <div class="post">
    [album stuff]
  <% else %>
    <hr />
    [duplicate album stuff]
  <% end %>
<% end %>
</div>

We could move this "album stuff" in a partial as I mention in the previous post, and this may be a good idea, but it's much simpler to just move the album stuff outside of the "if" condition.

<% for album in @albums %>
  <% if album == @albums.first %>
    <div class="post">
  <% else %>
    <hr />
  <% end %>
  [album stuff]
<% end %>
</div>

In the first condition you are starting a div element, but the ending element is outside of the loop (from what I can tell). The starting element should always be in the same logical condition as the ending element so you always know it will be output together - you don't want a hanging </div> tag if no albums were found for example. The simple solution is to move the starting element to the top:

<div class="post">
<% for album in @albums %>
  <% if album != @albums.first %>
    <hr />
  <% end %>
  [album stuff]
<% end %>
</div>

All of that album stuff really should go into a partial. If you call the partial "_album.rhtml" you can pass the album using the ":object" option.

<div class="post">
<% for album in @albums %>
  <% if album != @albums.first %>
    <hr />
  <% end %>
  <%= render :partial => 'album', :object => album %>
<% end %>
</div>

Things are looking prettier now that there's no duplication and the elements are lining up. But there's still one more thing we can do here. I haven't tested it, but I think it will work:

<div class="post">
<%= render :partial => 'album', :collection => @albums, :spacer_template => { :inline => '<hr />' } %>
</div>

That's it. This single line will do what the above code did. The ":collection" parameter will loop through the array and render the partial for each element, just like before. The spacer specifies what is rendered in between the partials.


Onto the album partial:

<div class = "floatRight">
<img src=<%=album.art%> width=150 height=150 align="left"/>
</div>
<h2><%=link_to album.title, album_url(:albumtitle => album.sanitized_title)%></h2>
<h4><%=album.artist.title%></h4>
<% unless album.review.nil? -%>
  <%=truncate(album.review, 400)%>
<%end%>
<%= link_to '[read more]',album_url(:albumtitle => album.sanitized_title)%>
<%= link_to "buy via AmpCamp", "http://www.ampcamp.com/basic_search_res … p;select=1"%>
<%if session['user'] != nil %>
  <%=link_to "edit", :controller => 'albums', :action => 'edit', :id => album.id%>
  <%= link_to "buy via AmpCamp", "http://www.ampcamp.com/basic_search_res … p;select=1"%>
<%end%>

You may want to use the image_tag helper for the image here.

<div class="floatRight">
  <%= image_tag album.art, :size => "150x150", :align => "left" %>
</div>

It's not much shorter, but considering you are using a variable for the image source I think it works better.

Looks like you're calling album_url a couple times here - I consider this duplication because there's some logic involved in setting the album title. Consider moving this into a helper method. Same goes for the ampcamp url further down.

# in helper
def url_for_album(album)
  album_url(:albumtitle => album.sanitized_title)
end

def ampcamp_url_for_album(album)
  "http://www.ampcamp.com/basic_search_res … p;select=1"
end


<h2><%= link_to album.title, url_for_album(album) %></h2>
...
<%= link_to '[read more]', url_for_album(album) %>
...
<%= link_to "buy via AmpCamp", ampcamp_url_for_album(album) %>

Moving on. You are doing a check for nil before displaying the review. I believe the truncate method already does a nil check so that isn't necessary.

You can use the logged_in? method we made earlier for the user check. You can also remove the duplicate "Buy via AmpCamp", I'm not sure why this is duplicated.

The end result looks like this:

# _album.rhtml partial
<div class="floatRight">
  <%= image_tag album.art, :size => "150x150", :align => "left" %>
</div>

<h2><%= link_to album.title, url_for_album(album) %></h2>
<h4><%= album.artist.title %></h4>

<%= truncate(album.review, 400) %>

<%= link_to '[read more]', url_for_album(album) %>
<%= link_to "buy via AmpCamp", ampcamp_url_for_album(album) %>
<%= link_to "edit", :controller => 'albums', :action => 'edit', :id => album if logged_in? %>


# index.rhtml template
<% if logged_in? %>
  <h2><u><%= link_to "New Album", :action => 'new'%></u></h2>
<% end %>

<hr />

<div class="post">
  <%= render :partial => 'album', :collection => @albums, :spacer_template => { :inline => '<hr />' } %>
</div>


This is less than half of what the other code was. Definitely a satisfying refactoring.

Railscasts - Free Ruby on Rails Screencasts

Re: Refactoring My Application

Oh my God, Ryan.
Tomorrow is going to be a fun day for me. I'm so stoked on this.
I really appreciate it.
I'm sure i'll be taking notes tomorrow while implementing.
ahhh!

Thanks.