3
\$\begingroup\$

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
\$\endgroup\$
4
  • \$\begingroup\$ out of curiosity: can you paste a URL that URI.parse does not like? \$\endgroup\$
    – tokland
    Commented Jul 24, 2013 at 12:33
  • \$\begingroup\$ URI.parse 'кококо' \$\endgroup\$
    – ujifgc
    Commented Jul 24, 2013 at 12:40
  • 1
    \$\begingroup\$ But URI is correct here, you have to escape it first (like any browser does): URI.parse(URI.escape('кококо')). \$\endgroup\$
    – tokland
    Commented Jul 24, 2013 at 12:51
  • \$\begingroup\$ Thank you, this makes sense. I updated my question with new code. \$\endgroup\$
    – ujifgc
    Commented Jul 24, 2013 at 17:46

1 Answer 1

4
\$\begingroup\$

Some notes:

  • url_replace( '/news' ): Each language has its formatting rules. In Ruby almost nobody inserts 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 accept 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
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the tips! I updated my code. I think, it's pretty straightforward and clean considering I like it imperative and tested. \$\endgroup\$
    – ujifgc
    Commented Jul 25, 2013 at 7:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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