CODEWITHSUNDEEP
ruby-on-railsruby
Ali Kazmi
Ali Kazmi

Reputation: 35

Optimize 'if else' conditions in rails

I am making an application, part of whose code requires many if .. else conditions:

   if @model_name == "Style"
        if row.include? ('colors')
          colors = row['colors'].split(';')
          model.style_colors.concat Color.where('code IN (?)', colors).map {|i| i.id.to_s }
          row.delete('colors')
        end

        if row.include? ('gender') and row.include? ('garments')
          @garments = row['garments']
          @gender = row['gender']


          row.delete('garments')
          row.delete('gender')
        end

        if row.include? ('sports')
          @sports = row['sports']

          row.delete('sports')
        end

        if row.include?('decoration_packages')
          @decorations_packages = row['decoration_packages']

          row.delete('decoration_packages')
        end

        model.attributes = row.to_hash.merge!(active: FALSE)
      else
        model.attributes = row.to_hash
      end

I need to make objects of row hash to access subclasses, and then delete them from row so it can be saved to a model.

Any idea how I can minimize the use of conditions or optimize it?

Upvotes: 0

Views: 769

Answers (3)

3limin4t0r
3limin4t0r

Reputation: 21130

I would do something like this with your current code:

if @model_name == "Style"
  row_key_set = row.keys.to_set

  if row.include? 'colors'
    colors = row['colors'].split(';')
    color_ids = Color.where(code: colors).pluck(:id)
    model.style_colors.concat(color_ids.map(&:to_s))
  end

  if row_key_set >= Set['gender', 'garments']
    @garments = row.delete('garments')
    @gender = row.delete('gender')
  end

  @sports = row.delete('sports')
  @decorations_packages = row.delete('decoration_packages')

  model.attributes = row.to_hash.merge(active: false)
else
  model.attributes = row.to_hash
end

  • Instead of using Color.where('code IN (?)', colors) you can just use Color.where(code: colors).

  • Instead of using .map {|i| i.id.to_s } you can use pluck (.pluck(:id)) to get an array of color ids. This also makes for a quicker query since only the ids get fetched from the database instead of the whole records.

  • I personally like to use sets to check if multiple values are present in another set. For this reason I create the row_key_set variable row.keys.to_set. Now you can easily check certain keys are present on your hash by just checking if the key set is greater or equal than another set (thus being a superset). row_key_set >= Set['gender', 'garments'] With just one check you could leave this out, but if you have multiple checks this might be worth the trouble. I also find code written this way also more readable, but that's just personal peference.

  • You don't need to check if a key is present on a Hash, the documentation tells us the following:

    Deletes the key-value pair and returns the value from hsh whose key is equal to key. If the key is not found, it returns nil.

    This means you can leave out the include? check and write the result from the delete directly to the instance variable. If the key is not present nil will be set for the instance variable.

  • Lastly I would leave out the explanation mark in row.to_hash.merge!(active: false). The version without explanation mark doesn't alter the original array and reduces the chance on accidental side effects. You're saving the variable to model.attributes anyway and toss away the generated array from the to_hash method. It's normally better to use non-altering versions of methods, unless you explicitly want a certain effect to happen.

Upvotes: 0

sawa
sawa

Reputation: 168101

if @model_name == "Style"
  if row.include?('colors')
    model.style_colors.concat(
      Color.where(code: row.delete('colors').split(';')).pluck(:id).map(&:to_s)
    )
  end
  if row.include?('gender') and row.include?('garments')
    @garments = row.delete('garments')
    @gender = row.delete('gender')
  end
  if row.include?('sports')
    @sports = row.delete('sports')
  end
  if row.include?('decoration_packages')
    @decorations_packages = row.delete('decoration_packages')
  end
  model.attributes = row.to_hash.merge!(active: false)
else
  model.attributes = row.to_hash
end

Upvotes: 0

AJFaraday
AJFaraday

Reputation: 2450

There's a few optimisations here...

row.include? ('gender') and row.include? ('garments')

could be implemented as

['gender', 'garments'].all?{|x| row.include?(x)}

@garments = row['garments']
row.delete('garments')

could be implemented as

@garments = row.delete('garments')

You could actually squash a lot of these onto one line:

if row.include? ('sports')
  @sports = row['sports']
  row.delete('sports')
end

could be 

@sports = row.delete('sports') if row.include? ('sports')

Also worth considering:

  • Do you need to delete the values from 'row'? Could you just retrieve the value?

  • What are you trying to do here? It looks like you're pulling a hash into instance variables... Which is what ActiveRecord does, basically. Could you just create a model with these attributes and then call it in this style?

    Style.new(row)

Upvotes: 3

Related Questions