HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Replace URL parameters with Ruby

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withreplacerubyparametersurl

Problem

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

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#28917, answer score: 4

Revisions (0)

No revisions yet.