extract private methods from complex controller actions, and reorganize.

app/controllers/calagator/venues_controller.rb

@@ -8,33 +8,35 @@ class VenuesController < Calagator::ApplicationController
   require_admin only: [:duplicates, :squash_many_duplicates]
 
   # GET /venues
-  # GET /venues.xml
   def index
     @search = Venue::Search.new(params.permit!)
     @venues = @search.venues
 
     flash[:failure] = @search.failure_message
     return redirect_to venues_path if @search.hard_failure?
+    render_venues @venues
+  end
 
+  def render_venues venues
     respond_to do |format|
       format.html # index.html.erb
       format.kml  # index.kml.erb
-      format.xml  { render :xml  => @venues }
-      format.json { render :json => @venues, :callback => params[:callback] }
-      format.js   { render :json => @venues, :callback => params[:callback] }
+      format.xml  { render xml:  venues }
+      format.json { render json: venues, callback: params[:callback] }
+      format.js   { render json: venues, callback: params[:callback] }
     end
   end
+  private :render_venues
 
+  # GET /autocomplete via AJAX
   def autocomplete
     @venues = Venue
       .non_duplicates
       .in_business
       .where(["LOWER(title) LIKE ?", "%#{params[:term]}%".downcase])
       .order('LOWER(title)')
 
-    respond_to do |format|
-      format.json { render :json => @venues, :callback => params[:callback] }
-    end
+    render json: @venues, callback: params[:callback]
   end
 
   # GET /venues/map
@@ -43,94 +45,121 @@ def map
   end
 
   # GET /venues/1
-  # GET /venues/1.xml
-  def show
-    @venue = Venue.find(params[:id])
+  before_action :show_all_if_not_found, :ensure_progenitor, only: :show
+
+  def show_all_if_not_found
+    venue
+  rescue ActiveRecord::RecordNotFound => e
+    flash[:failure] = e.to_s
+    redirect_to venues_path
+    false
+  end
+  private :show_all_if_not_found
+
+  def ensure_progenitor
+    return unless venue.duplicate?
+    redirect_to venue.duplicate_of
+    false
+  end
+  private :ensure_progenitor
 
-    return redirect_to @venue.duplicate_of if @venue.duplicate?
+  def show
+    render_venue venue
+  end
 
+  def render_venue venue
     respond_to do |format|
       format.html
-      format.xml  { render xml: @venue }
-      format.json { render json: @venue, callback: params[:callback] }
-      format.ics  { render ics: @venue.events.order("start_time ASC").non_duplicates }
+      format.xml  { render xml: venue }
+      format.json { render json: venue, callback: params[:callback] }
+      format.ics  { render ics: venue.events.order("start_time ASC").non_duplicates }
     end
-
-  rescue ActiveRecord::RecordNotFound => e
-    flash[:failure] = e.to_s
-    redirect_to venues_path
   end
+  private :render_venue
 
   # GET /venues/new
-  # GET /venues/new.xml
   def new
-    @venue = Venue.new
+    venue
     render layout: params[:layout] != "false"
   end
 
   # GET /venues/1/edit
   def edit
-    @venue = Venue.find(params[:id])
+    venue
   end
 
-  # POST /venues
-  # POST /venues.xml
-  def create
-    @venue = Venue.new
-    create_or_update
-  end
+  # POST /venues, # PUT /venues/1
+  before_action :prevent_evil_robots, only: [:create, :update]
 
-  # PUT /venues/1
-  # PUT /venues/1.xml
-  def update
-    @venue = Venue.find(params[:id])
-    create_or_update
+  def prevent_evil_robots
+    return unless params[:trap_field].present?
+    flash[:failure] = "<h3>Evil Robot</h3> We didn't save this venue because we think you're an evil robot. If you're really not an evil robot, look at the form instructions more carefully. If this doesn't work please file a bug report and let us know."
+    render_failure venue
+    false
   end
+  private :prevent_evil_robots
 
   def create_or_update
-    @venue.attributes = params.permit![:venue] || {}
-    respond_to do |format|
-      if !evil_robot? && @venue.save
-        format.html { redirect_to from_event || @venue, flash: { success: "Venue was successfully saved." } }
-        format.xml  { render xml: @venue, status: :created, location: @venue }
-      else
-        format.html { render action: @venue.new_record? ? "new" : "edit" }
-        format.xml  { render xml: @venue.errors, status: :unprocessable_entity }
-      end
+    venue.attributes = params.permit![:venue] || {}
+    if venue.save
+      render_success venue
+    else
+      render_failure venue
     end
   end
 
-  # DELETE /venues/1
-  # DELETE /venues/1.xml
-  def destroy
-    @venue = Venue.find(params[:id])
-
-    if @venue.events.any?
-      message = "Cannot destroy venue that has associated events, you must reassociate all its events first."
-      respond_to do |format|
-        format.html { redirect_to @venue, flash: { failure: message } }
-        format.xml  { render xml: message, status: :unprocessable_entity }
-      end
-    else
-      @venue.destroy
-      respond_to do |format|
-        format.html { redirect_to venues_path, flash: { success: %("#{@venue.title}" has been deleted) } }
-        format.xml  { head :ok }
-      end
+  alias_method :create, :create_or_update
+  alias_method :update, :create_or_update
+
+  def render_success venue
+    respond_to do |format|
+      format.html { redirect_to from_event || venue, flash: { success: "Venue was successfully saved." } }
+      format.xml  { render xml: venue, status: :created, location: venue }
     end
   end
+  private :render_success
 
-  private
-
-  def evil_robot?
-    if params[:trap_field].present?
-      flash[:failure] = "<h3>Evil Robot</h3> We didn't save this venue because we think you're an evil robot. If you're really not an evil robot, look at the form instructions more carefully. If this doesn't work please file a bug report and let us know."
+  def render_failure venue
+    respond_to do |format|
+      format.html { render action: venue.new_record? ? "new" : "edit" }
+      format.xml  { render xml: venue.errors, status: :unprocessable_entity }
     end
   end
+  private :render_failure
 
   def from_event
     Event.find_by_id(params[:from_event])
   end
+  private :from_event
+
+  # DELETE /venues/1
+  before_action :prevent_destruction_of_venue_with_events, only: :destroy
+
+  def prevent_destruction_of_venue_with_events
+    return if venue.events.none?
+
+    message = "Cannot destroy venue that has associated events, you must reassociate all its events first."
+    respond_to do |format|
+      format.html { redirect_to venue, flash: { failure: message } }
+      format.xml  { render xml: message, status: :unprocessable_entity }
+    end
+    false
+  end
+  private :prevent_destruction_of_venue_with_events
+
+  def destroy
+    venue.destroy
+    respond_to do |format|
+      format.html { redirect_to venues_path, flash: { success: %("#{venue.title}" has been deleted) } }
+      format.xml  { head :ok }
+    end
+  end
+
+  private
+
+  def venue
+    @venue ||= params[:id] ? Venue.find(params[:id]) : Venue.new
+  end
 end
 
 end

spec/controllers/calagator/venues_controller_spec.rb

@@ -8,23 +8,20 @@ module Calagator
 
   render_views
 
-  it "should redirect duplicate venues to their master" do
-    venue_master = FactoryGirl.create(:venue)
-    venue_duplicate = FactoryGirl.create(:venue)
-
-    # No redirect when they're unique
-    get 'show', :id => venue_duplicate.id
-    expect(response).not_to be_redirect
-    expect(assigns(:venue).id).to eq venue_duplicate.id
-
-    # Mark as duplicate
-    venue_duplicate.duplicate_of = venue_master
-    venue_duplicate.save!
-
-    # Now check that redirection happens
-    get 'show', :id => venue_duplicate.id
-    expect(response).to be_redirect
-    expect(response).to redirect_to(venue_url(venue_master.id))
+  context "concerning duplicates" do
+    let!(:venue_master) { FactoryGirl.create(:venue) }
+    let!(:venue_duplicate) { FactoryGirl.create(:venue, duplicate_of: venue_master) }
+
+    it "redirects duplicate venues to their master" do
+      get 'show', id: venue_duplicate.id
+      expect(response).to redirect_to(venue_url(venue_master.id))
+    end
+
+    it "doesn't redirect non-duplicates" do
+      get 'show', id: venue_master.id
+      expect(response).not_to be_redirect
+      expect(assigns(:venue).id).to eq venue_master.id
+    end
   end
 
   context "with admin auth for duplicates" do