current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite
1

I have a method to replace URL parameters in an URL. It receives url as mandatory parameter and prefix and/or hash as optional parameters. Examples:

url_replace( '/news?b=2', { b: nil } )                     # => '/news'
url_replace( '/news?b=2', { b: 3 } )                       # => '/news?b=3'
url_replace( '/news?a=b', '/bar' )                         # => '/bar?a=b'
url_replace( '/news?a=b&c=d', '/bar', c: nil )             # => '/bar?a=b'

The method:

def url_replace( target, *args )
  uri = URI.parse(URI.escape target)
  if hash = args.last.kind_of?(Hash) && args.last
    query = uri.query ? CGI.parse(uri.query) : {}
    hash.each do |k,v|
      v ? query[k.to_s] = v.to_s : query.delete(k.to_s)
    end
    uri.query = query.any? ? query.map{|k,v| "#{CGI.escape k.to_s}=#{CGI.escape Array(v).join}"}.join('&') : nil
  end
  prefix = args.first.kind_of?(String) && args.first
  uri.path = CGI.escape(prefix)  if prefix
  CGI.unescape(uri.to_s)
end

I would like some refactoring or speed optimizations.

Okay, here's the code I ended up with:

def url_replace( target, *args )
  uri = URI.parse(URI::DEFAULT_PARSER.escape target)
  uri.path = CGI.escape(args.first)  if args.first.kind_of?(String)
  if args.last.kind_of?(Hash)
    query = uri.query ? CGI.parse(uri.query) : {}
    args.last.each{ |k,v| v ? query[k.to_s] = v.to_s : query.delete(k.to_s) }
    uri.query = query.any? ? URI.encode_www_form(query) : nil
  end
  CGI.unescape(uri.to_s)
end
share|improve this question
    
out of curiosity: can you paste a URL that URI.parse does not like? –  tokland Jul 24 '13 at 12:33
    
URI.parse 'кококо' –  ujifgc Jul 24 '13 at 12:40
1  
But URI is correct here, you have to escape it first (like any browser does): URI.parse(URI.escape('кококо')). –  tokland Jul 24 '13 at 12:51
    
Thank you, this makes sense. I updated my question with new code. –  ujifgc Jul 24 '13 at 17:46

1 Answer 1

up vote 2 down vote accepted

Some notes:

  • url_replace( '/news' ): Each language has its formatting rules. In Ruby almost nobody puts those spaces after and before parens.
  • hash = args.last.kind_of?(Hash) && args.last: I'd strongly discourage this kind of positional arguments, the method signature is severely impaired. Use an options hash instead (note that Ruby 2.0 finally provides keyword arguments).
  • query.delete(k.to_s). If you check my other answers you'll see I tend to favour functional programming, so I'd rewrite this using expressions instead of statements. Code is much more clean when they say what things are instead of how you change their value.
  • Uses of args.first in the middle of the code: Strive for declarative code, give names to things before you use them when it's not clear what they are.
  • I'd admit only strings as keys for the query string, or, if Activesupport is at hand, I'd call stringify_keys or Hash[h.map { |k, v| [k.to_s, v] }] at some point. This way I'd avoid mixing symbols and strings.

I'd write:

require 'uri'
require 'cgi'

def url_replace(url, options = {})
  uri = URI.parse(URI.encode(url))
  hquery = CGI::parse(uri.query)
  components = Hash[uri.component.map { |key| [key, uri.send(key)] }]
  new_hquery = hquery.merge(options[:merge_query] || {}).select { |k, v| v }
  new_query = URI.encode_www_form(new_hquery)
  new_components = {path: options[:path] || uri.path, query: new_query}
  new_uri = URI::Generic.build(components.merge(new_components))
  URI.decode(new_uri.to_s)
end

puts url_replace('/news?a=b&c=d', path: '/bar', merge_query: {"c" => nil})
#=> /bar?a=b
share|improve this answer
    
Thanks for the tips! I updated my code. I think, it's pretty straightforward and clean considering I like it imperative and tested. –  ujifgc Jul 25 '13 at 7:14

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.