2
\$\begingroup\$

As part of learning Rails framework, I have small app in progress. Basically idea is, user can create IP address, define is it in use, to which server is assigned, which user is it using and role of IP address in web hosting environment. I have 3 models, IpAddress, IpAddressRole and IpAddressUser. User is able to define IpAddressRole and IpAddressUser from prepopulated postgresql database.

Main controller is IpAddressesController, which is responsible for creating, showing and other CRUD operations on primary model.

ip_addresses_controller.rb code is here:

class IpAddressesController < ApplicationController

    def index
        @ip_addresses = IpAddress.all.order(:title)
    end

    def show
        @ip_address = IpAddress.find(params[:id])
    end

    def new
        @ip_address_roles = IpAddressRole.all
        @servers = Server.all
        @ip_address_users = IpAddressUser.all
    end

    def create
        @ip_address_roles = IpAddressRole.all
        @servers = Server.all
        @ip_address_users = IpAddressUser.all

        @ip_address = IpAddress.new(ip_address_params)
        @ip_address.save
        redirect_to ip_addresses_path
        #render plain: params[:ip_address].inspect
    end

    def destroy
        @ip_address = IpAddress.find(params[:id])
        @ip_address.destroy
        flash[:success] = "Record successfully deleted"
        redirect_to ip_addresses_path
    end

    private 
        def ip_address_params
            params.require(:ip_address).permit(:title, :server, :user, :is_in_use, :role)
        end
end

And ip_address.rb code is here:

class IpAddress < ActiveRecord::Base
    def is_ip_address_in_use
        if self.is_in_use == 'true' 
            self.is_in_use = 'yes'
        else
            self.is_in_use = 'no'
        end
    end


end

I have some great resources near hand, but my controller is getting fat, and model skinny, also, in my controller I have a lot of repetition. How can I refactor model and controller? Controller is really ugly.

\$\endgroup\$
2
\$\begingroup\$

First of all you should look at your controller and see what common functionality you can extract to a private methods and callbacks.

Another fairly large concern with you controller is that you are just assuming that a record is created or deleted without checking the return value. Create / Update / Destroy actions can and will fail.

class IpAddressesController < ApplicationController

    before_filter :set_ip_address, only: [:show, :edit, :update, :destroy]

    def index
        @ip_addresses = IpAddress.all.order(:title) # extract to model default scope?
    end

    def show
    end

    def new
        load_form_resources
        @ip_address = IpAddress.new
    end

    def create
        @ip_address = IpAddress.new(ip_address_params)
        if @ip_address.save 
            redirect_to ip_addresses_path
        else
            load_form_resources
            render :new
        end
    end

    def destroy
        # Remember that destroy can actually fail! 
        if @ip_address.destroy
            flash.success "Record successfully deleted"
            redirect_to ip_addresses_path
        else
            flash.alert "Record not deleted"
        end
    end

    private 

        def set_ip_address
          @ip_address = IpAddress.find(params[:id])
        end 

        def load_form_resources
            @ip_address_roles = IpAddressRole.all
            @servers = Server.all
            @ip_address_users = IpAddressUser.all
        end

        def ip_address_params
            params.require(:ip_address).permit(:title, :server, :user, :is_in_use, :role)
        end
end

When it comes to your model, then I usually preach so much against boolean state flags that I feel somewhat like a broken record.

Instead of having a ip_addresses.is_in_use column you could add a ip_addresses.state integer column.

Rails has a really fancy trick called enums.

class IpAddress < ActiveRecord::Base
  enum state: [:available, :in_use] 
end

Which gives you:

IpAddress.available # gets all free ip adresses
IpAddress.in_use # gets all in_use ip adresses
@ip_adress.available? # check status
@ip_adress.in_use?
@ip_adress.in_use! # setters
\$\endgroup\$
  • \$\begingroup\$ Should I, or can I put '@servers' and '@ip_address_users' instances just once, before all CRUD methods in controller, so I can use collection in my ip_address views? Or will this put more overhead on DB? \$\endgroup\$ – Alan Kis Aug 23 '15 at 11:49
  • 1
    \$\begingroup\$ Don't load anything unless you absolutely need it. \$\endgroup\$ – papirtiger Aug 23 '15 at 12:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.