Daily Refactor #73: Move Method to before_filter in InvoiceController

I’m starting to refactor my Redmine Invoice plugin now. This plugin is over two years old now and it’s code has seen better days.

The Refactoring

The Rails Best Practices gem showed me a quick refactoring to remove some duplication from InvoiceController using a before_filter.

Before

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29

class InvoiceController  'applied_on DESC')
    render :layout => 'print' if params[:print]
  end
 
  def edit
    @invoice = Invoice.find(params[:invoice][:id])
    @last_number = last_invoice_number
  end
  def update
    @invoice = Invoice.find(params[:invoice][:id])
    if @invoice.update_attributes(params[:invoice])
      flash[:notice] = "Invoice saved"
      redirect_to :action => "show", :id => params[:id], :invoice => { :id => @invoice }
    else
      render :action => 'edit', :id => params[:id], :invoice => { :id => @invoice }
    end    
  end
 
  def destroy
    @invoice = Invoice.find(params[:invoice][:id])
    if @invoice.destroy
      flash[:notice] = "Invoice deleted"
      redirect_to :action => "index", :id => params[:id]
    else
      flash[:notice] = "Error"
      render :action => 'index', :id => params[:id]
    end
  end
end

After

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35

class InvoiceController  [:show, :edit, :update, :destroy]
 
  def show
    @payments = @invoice.payments.find(:all, :order => 'applied_on DESC')
    render :layout => 'print' if params[:print]
  end
 
  def edit
    @last_number = last_invoice_number
  end
 
  def update
    if @invoice.update_attributes(params[:invoice])
      flash[:notice] = "Invoice saved"
      redirect_to :action => "show", :id => params[:id], :invoice => { :id => @invoice }
    else
      render :action => 'edit', :id => params[:id], :invoice => { :id => @invoice }
    end    
  end
 
  def destroy
    if @invoice.destroy
      flash[:notice] = "Invoice deleted"
      redirect_to :action => "index", :id => params[:id]
    else
      flash[:notice] = "Error"
      render :action => 'index', :id => params[:id]
    end
  end
 
  private
  def find_invoice
    @invoice = Invoice.find(params[:invoice][:id])
  end
end

Review

I can see the Redmine Invoice plugin is going to need a lot of work over the next few days. It has a lot of legacy code back from Redmine’s early days, before I figured out how to test Redmine plugins. I’m hoping to get it cleaned up and released along with Redmine 1.0 this summer.

Reference commit