CODEWITHSUNDEEP
ruby-on-railsrubyruby-on-rails-3
Matt Elhotiby
Matt Elhotiby

Reputation: 44066

Is there a more ruby way of doing this

Ok so i have this helper

def current_company_title
 (Company.find_by_id(params["company_id"]).name rescue nil) || (@companies.first.name rescue nil) current_user.company.name
end

Basically what I am achieving with this is the following ...

If the param["company_id"] exists then try to get the company and if not then if @companies exists grab the first company name and if not then get the current users company name

This works but the rescues seem like a hack...any idea on another way to achieve this

Upvotes: 2

Views: 98

Answers (6)

rubiii
rubiii

Reputation: 6983

Less "magic", simple code, simple to read:

def current_company_title
 company = Company.where(id: params["company_id"]).presence
 company ||= @companies.try(:first)
 company ||= current_user.company
 company.name
end

Ps. Not a big fan of Rails' try method, but it solves the problem.

Upvotes: 2

Pavling
Pavling

Reputation: 3963

The rescues are a hack, and will obscure other errors if they occur.

Try this:

(Company.find_by_id(params["company_id"].name if Company.exists?(params["company_id"]) ||
(@companies.first.name if @companies && @companies.first) || 
current_user.company.name

then you can extract each of the bracketed conditions to their own methods to make it more readable, and easier to tweak the conditions:

company_name_from_id(params["company_id"]) || name_from_first_in_collection(@companies) || current_user_company_name

def company_name_from_id(company_id)
  company=Company.find_by_id(company_id)
  company.name if company
end

def name_from_first_in_collection(companies)
  companies.first.name if companies && companies.first
end

def current_user_company_name
  current_user.company.name if current_user.company
end

Upvotes: 1

ragingsquirrel3
ragingsquirrel3

Reputation: 425

def current_company_title
    if params["company_id"]
        return Company.find_by_id(params["company_id"]).name
    elsif @companies
        return @companies.first.name
    else
        return current_user.company.name
    end
end

Upvotes: 1

ksol
ksol

Reputation: 12235

Company.find_by_id(params["company_id"]).name`

find and its derivates are meant to be used when you're sure-ish you'll have a positive result, and only in some cases (row was deleted, etc) errors. That's why it raises an exception. In your case, you're assuming it's gonna fail, so a regular where, which would return nil if no rows was found, would do better, and remove the first rescue

@companies.first.name rescue nil

could be replaced by

@companies.first.try(:name)

I'll let you check the api for more on the topic of try. It's not regular ruby, it's a Rails addition.

Upvotes: 2

lwe
lwe

Reputation: 2625

Indeed rescue is kind of a hack, id' probably split it up into two methods and then use try to fetch the name if available: http://api.rubyonrails.org/classes/Object.html#method-i-try

def current_company
  @current_company ||= Company.find_by_id(params[:company_id]) || @companies.try(:first) || current_user.try(:company)
end

def current_company_name
  current_company.try(:name)
end

Upvotes: 3

DigitalRoss
DigitalRoss

Reputation: 146073

[Company.find_by_id(params["company_id"]),
  @companies.to_a.first,
  current_user.company
].compact.first.name

Upvotes: 0

Related Questions