Is there a way to refactor this code?

Question

I'm new to ruby on rails, on this project I'm using Ruby 2.0 and Ruby on Rails 3.0.

I would like to know if this piece of code can be refactored, as it is

unless params["ot_code"].nil?       
  ots = params["ot_code"].gsub(/\r\n?/, "").gsub(";","','").upcase
  ots[ots.length,1] = "'"
  ots =  ots.rjust(ots.length+1,"'")
end

unless params["circuit_id_multiple"].nil?
   multiple_circuit = params["circuit_id_multiple"].gsub(/\r\n?/, "").gsub(";","','")
   multiple_circuit[multiple_circuit.length,1] = "'"
   multiple_circuit = multiple_circuit.rjust(multiple_circuit.length+1,"'")
end

unless params["multiple_element_code"].nil?
   multiple_element_code = params["multiple_element_code"].gsub(/\r\n?/, "").gsub(";","','")
   multiple_element_code[multiple_element_code.length,1] = "'"
   multiple_element_code = multiple_element_code.rjust(multiple_element_code.length+1,"'")
end

Answer 1

I'm a little worried about your gsubs, but assuming this is what you want to do:

def replace_and_wrap(str)
  return nil if str.nil?
  %Q{'#{str.gsub(/\r\n?/, "").gsub(";","','")}'}  
end

ots = replace_and_wrap(params["ot_code"])
multiple_circuit = replace_and_wrap(params["circuit_id_multiple"])
multiple_element_code = replace_and_wrap(params["multiple_element_code"])

Note that this changes the behavior of your code slightly: variables (like ots) are set to nil if the param is nil, rather than remaining undefined. Depending on how you are using these variables, this behavior is probably better anyway.

Answer 2

Some notes:

• params["ot_code"]. Is this a Rails controller? Then the first advice would be: never, ever write such long code in controllers. Controllers methods should be very lean, they should deal with the very minimal calls to models, flash setting and other render details. Nothing more.

• unless params["ot_code"].nil?. Try to use positive logic (and use symbols, it's more idiomatic): if params[:ot_code] or if params[:ot_code].present? depending whether you want to consider empty strings or not.

• There is almost exactly the same code repeated 3 times, that needs some DRYing. The two typical approaches: 1) bottom-up: abstract to function/method, or 2) top-bottom: write a loop. The first approach was already covered by @kardeiz, so I'll show the second:

  keys = [:ot_code, :circuit_id_multiple, :multiple_element_code]
  ots, multiple_circuit, multiple_element_code = keys.map do |key|
    if params[key].present?
      "'%s'" % params[key].gsub(/\r\n?/, '').gsub(/;/, '')
    end
  end