HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Cleaning up a multi-model view in Rails3

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
multiviewrails3cleaningmodel

Problem

I'm new to rails and have built a multi-model form. It's working, but some of the code doesn't feel right.

The model code is below. I'm using Devise for my user authentication:

# models/signup.rb
class Signup < ActiveRecord::Base
  has_one :company
  accepts_nested_attributes_for :company
end

# models/company.rb
class Company < ActiveRecord::Base
  belongs_to  :signup
  has_many    :users

  accepts_nested_attributes_for :users
end

# models/user.rb
class User < ActiveRecord::Base
  devise :database_authenticatable, :confirmable, :recoverable, 
    :rememberable, :trackable, :validatable, :registerable

  attr_accessible :email, :password, :password_confirmation

  belongs_to :company
end


My view is fairly standard and follows the typical pattern for multiple models. The most important code is the initial Signup creation which is done via the following helper:

# signup is created via signups#new 
def setup_signup(signup)
  signup.company ||= Company.new
  if signup.company.users[0] == nil
    1.times { signup.company.users.build }
  end
  signup
end


I had to add a condition to not create additional users, because if the form validation failed it would create another user.

Finally my controller code looks like this:

# controllers/signups_controller.rb
class SignupsController  "new" 
    end
  end
end


This is what feels wrong to me:

  • When signing up to my website, there will only ever be a single User created. However, because a user is associated via a Company, and a Company has_many users I have to access this user in an awkward manner: signup.company.users.first. Obviously I could create an instance variable user = signup.company.users.first, and if that's the best solution, I'll take it, but it feels like there should be a better solution.



  • The setup_signup helper method requires checking if a user has already been created so a second user isn't created: if signup.company.users[0] == nil. To me if feels like Rails

Solution

Firstly (and yes I did read the comments) the signup model seems unessential.

So firstly we should do away with that, if you need to associate payment details then that could easily be done directly on the company model.

Secondly you controller is getting a bit cuddly. In favour of having "fat model, skinny controller" let's move all that signup logic into the company model in the form of a Company::register method.

So far we have changes like this:

# app/models/company.rb
class Company < ActiveRecord::Base
  has_many :users
  accepts_nested_attributes_for :users

  def self.register(params)
    company = self.new(params)

    company.users.first.admin = true
    company.users.first.skip_confirmation!

    if company.save
      company
    else
      false
    end
  end
end


Now your controller can look like this:

# app/controllers/companies_controller.rb
class CompaniesController < ApplicationController

  def new
    @company = Company.new
  end

  def create
    if @company = Company.register(params[:company])
      sign_in_and_redirect(@company.users.first)
    else
      render :new
    end
  end
end


I'm also pretty sure that it's unessential to use that helper you have above. The form builder should be able to work it out from the model accepting nested attributes.

Code Snippets

# app/models/company.rb
class Company < ActiveRecord::Base
  has_many :users
  accepts_nested_attributes_for :users

  def self.register(params)
    company = self.new(params)

    company.users.first.admin = true
    company.users.first.skip_confirmation!

    if company.save
      company
    else
      false
    end
  end
end
# app/controllers/companies_controller.rb
class CompaniesController < ApplicationController

  def new
    @company = Company.new
  end

  def create
    if @company = Company.register(params[:company])
      sign_in_and_redirect(@company.users.first)
    else
      render :new
    end
  end
end

Context

StackExchange Code Review Q#1804, answer score: 5

Revisions (0)

No revisions yet.