current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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
share|improve this question

2 Answers 2

up vote 2 down vote

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.

share|improve this answer
    
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. –  tokland Aug 13 '13 at 16:51
    
Thanks @tokland, I didn't realize that. –  kardeiz Aug 13 '13 at 17:53
    
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. –  tokland Aug 13 '13 at 18:19
    
str && "'#{str.gsub(/\r\n?/, "").gsub(";","','")}'" –  Nakilon Aug 14 '13 at 15:29
up vote 2 down vote

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
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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