Ruby If statement refactor

Question

I'm coming back to Ruby after a long time away.

I've written the following code which is functional:

def generate_address_record(data)
 address = FM[:fap_address].build do |a|
        if data['Line 1'].blank?
          a.unstructured_address.line1 = nil
        else
          a.unstructured_address.line1 = data['Line 1']
        end

        if data['Line 2'].blank?
          a.unstructured_address.line2 = nil
        else
          a.unstructured_address.line2 = data['Line 2']
        end

        if data['Line 3'].blank?
          a.unstructured_address.line3 = nil
        else
          a.unstructured_address.line3 = data['Line 3']
        end

        if data['Line 4'].blank?
          a.unstructured_address.line4 = nil
        else
          a.unstructured_address.line4 = data['Line 4']
        end

        if data['Line 5'].blank?
          a.unstructured_address.line5 = nil
        else
          a.unstructured_address.line5 = data['Line 5']
        end

        if data['Postcode'].blank?
          a.unstructured_address.postcode = nil
        else
          a.unstructured_address.postcode = data['Postcode']
        end
  end
end

Is there a way this can be re-written in a 'nicer' way in one loop so that I don't need all of these individual if statements.

Any advice would be greatly appreciated.

There is no such method as blank? in Ruby.

Aleksei Matiushkin
– Aleksei Matiushkin

2019-08-23 08:12:52 +00:00
Commented Aug 23, 2019 at 8:12 — Aleksei Matiushkin
– Aleksei Matiushkin, Commented Aug 23, 2019 at 8:12

Fabio · Accepted Answer · 2019-08-23 10:22:23Z

2

If data contains only expected values, you can convert key to the method name.

def generate_address_record(data)
  address = FM[:fap_address].build do |a|
      data.each do |key, value|
          name = key.gsub(/[[:space:]]/, '').downcase
          a.unstructured_address.public_send("#{name}=", value.presence)
      end
  end
end

Be careful(or don't use this approach) when hash keys are coming from outside of the application or other uncontrolled environment.

edited Aug 23, 2019 at 10:22

answered Aug 23, 2019 at 8:18

Fabio

32.5k4 gold badges35 silver badges77 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Jörg W Mittag Over a year ago

I generally prefer public_send over send. send does two things: send a message with a dynamic name and ignore access restrictions. In this case, you only need the former, so you can use public_send, and future readers of your code don't have to wonder why you are circumventing access restrictions.

Pascal Over a year ago

don't use send, use public_send as Jörg already mentioned. Also I would not recommend this approach, just go with what spickerman recommended. It is much easier to understand and extend. There will be no security problems (what if a field is called destroy) either.

Fabio Over a year ago

@Pascal, I think quite unlikely the setter destroy= will destroy the object. On other hand as I mentioned in the answer, use it if you are sure that hash keys are always same as expected class attributes. Of course you shouldn't use this where hash keys are passed from the uncontrolled environment. But that exactly what Rails do with request parameters when passing it straight to the model builder "yak" ;)

Pascal Over a year ago

ah true, the =, missed. You have to permit params in Rails when passing them into a model.

mrzasa · Accepted Answer · 2019-08-23 08:28:09Z

2

Yup, you can use #presence (I'm assuming you're using Rails):

a.unstructured_address.line1 = data['Line 1'].presence

#presence behaviour:

''.presence
# => nil
nil.presence
# => nil
'a'.presence
# => "a"
false.presence
# => nil

edited Aug 23, 2019 at 8:28

answered Aug 23, 2019 at 8:04

mrzasa

23.4k11 gold badges60 silver badges96 bronze badges

4 Comments

Marek Lipka Over a year ago

You don't need || nil, data['Line 2'].presence is enough.

Tom Over a year ago

.presence will assign to nil if blank? Excellent, thank you

Marek Lipka Over a year ago

@Tom yes, look at its source: apidock.com/rails/Object/presence

Mark Over a year ago

This is the nicest and most easily read solution in my opinion

spickermann · Accepted Answer · 2019-08-23 09:21:19Z

2

I propose this combination of the various already posted solutions because I think it has a good balance between shortness and readability:

def generate_address_record(data)
  address = FM[:fap_address].build do |a|
    a.unstructured_address.line1    = data['Line 1'].presence
    a.unstructured_address.line2    = data['Line 2'].presence
    a.unstructured_address.line3    = data['Line 3'].presence
    a.unstructured_address.line4    = data['Line 4'].presence
    a.unstructured_address.line5    = data['Line 5'].presence
    a.unstructured_address.postcode = data['Postcode'].presence
  end
end

answered Aug 23, 2019 at 9:21

spickermann

108k9 gold badges115 silver badges147 bronze badges

2 Comments

Pascal Over a year ago

This is much better than using send (or better public_send) just to save some lines of code.

Pascal Over a year ago

you could remove the assignment address = . And pass the unstructured_address in the block directly, so the a. can be removed.

max pleaner · Accepted Answer · 2019-08-23 08:18:17Z

0

One pattern I find usefull is to make a hash and then iterate over it:

def generate_address_record(data)
   address = FM[:fap_address].build do |a|
     {
       "Line 1" =>  :line1, 
       "Line 2" =>  :line2,
       "Line 3" =>  :line3,
       "Line 4" =>  :line4,
       "Line 5" =>  :line5,
       "Postcode" => :postcode
     }.each do |key, accessor|
       if data[key].blank?
         a.unstructured_address.send(accessor) = nil
       else
         a.unstructured_address.send(:"#{accessor}=") = data[key]
       end
     end
  end
end

You can also use this with presence as mrzasa shared

answered Aug 23, 2019 at 8:18

max pleaner

26.9k10 gold badges73 silver badges132 bronze badges

Collectives™ on Stack Overflow

Ruby If statement refactor

4 Answers 4

4 Comments

4 Comments

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

4 Comments

4 Comments

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related