4
\$\begingroup\$
if params[:package]
  city_ids = []

  city = params[:package].delete 'city'
  city_ids << city  if city.to_i > 0

  cities = params[:package].delete 'cities'
  city_ids += cities  if cities.class == Array && cities.length > 0

  params[:package][:cities] = city_ids.map{ |c| City.get(c) }.compact
end
\$\endgroup\$
3
\$\begingroup\$

This code looks pretty clean to me. Here are a few thoughts:

I'm not sure what your ORM is (or even that City is database backed), but most have a way to fetch more then one record at a time. Making a single call will save time and Database resources.

Another thought is that you are inducing a lot of local vars: city_ids, city, cities. What about trying to build up the array of ids in params[:package][:cities]? e.g.

#if cities is not an array, it is bad date get rid of it.
params[:package].delete 'cities' unless params[:package][:cities].kind_of? Array
params[:package][:cities] ||= []
params[:package][:cities] << (params[:package].delete 'city') if params[:package][:cities].to_i > 0
params[:package][:cities] = params[:package][:cities].map{ |c| City.get(c) }.compact

The code above doesn't seem very DRY because it has all this params[:package] stuff. If you do things like this a lot (your question makes it sound like you do) factor the operation out into a separate method:

def extract_nested_reference(hash,values) # change name as appopriate
  if hash
    hash.delete values unless hash[values].kind_of? Array
    hash[values] ||=[]
    hash[values] << hash[values.singularize] if hash[values.singularize].to_i > 0

    hash[values] = hash[values].map do |x|
      Object.const_get(values.singularize.constantize).get(c) 
    end.compact
  end
end

This method could then be called with extract_nested_reference(params[:package],'cities'). Refactoring like this will also encourage you to be consistent in how you build you params hash, and allow you to reuse code.

Finally, your question asked about better ways of dealing with hashes. I don't think there is. You will find code very similar to this living in most popular gems. AS you say, it is kind of clunky, and therefore and excellent candidate for a code redactor.

\$\endgroup\$
1
  • \$\begingroup\$ You are right about ORM. I'm usind marvellous DataMapper and it invokes just one sql statement instead of city_ids.count with this code: City.all(:id => city_ids.compact) \$\endgroup\$ – ujifgc Apr 21 '11 at 19:40

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