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,"'")

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,"'")

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,"'")

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(";","','")}'}  

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.

  • \$\begingroup\$ Actually a variable set inside a conditional is created (with value nil) even if the branch is not executed, so it's the same behavior. Not that's good practice though. \$\endgroup\$ – tokland Aug 13 '13 at 16:51
  • \$\begingroup\$ In fact, that's a good sign, conceptually it makes no sense :-). +1 btw. I would write str ? expression_using_str : nil but it's a minor detail. \$\endgroup\$ – tokland Aug 13 '13 at 18:19
  • \$\begingroup\$ str && "'#{str.gsub(/\r\n?/, "").gsub(";","','")}'" \$\endgroup\$ – Nakilon Aug 14 '13 at 15:29

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(/;/, '')

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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