Changing URLs into HTML links

Question

This method changes all URLs in the string into an HTML link. The goal is to not mutate the string passed in, which I believe is a good practice.

The new_string code in this method does not seem idiomatic.

def autolink_string string
  return    if string.nil? 
  return '' if string.empty?

  regex = /(\S+\.(com|net|org|edu|gov)(\/\S+)?)/
  groups = string.scan regex
  matches = groups.map &:first
  new_string = string.clone

  matches.each do |match|
    new_string.gsub! match, "<a href='http://#{ match }'>#{ match }</a>"
  end

  new_string
end

How can I refactor to remove the new_string? Any other suggestions?

Answer 1

Some notes:

• About the double return at the beginning: note that you could write return string if !string || string.empty?. In any case, I wouldn't check this, if the caller sends a nil as argument it's his/her fault. IMO in dynamic languages it does not make much sense to check the types of the arguments.

• Again, about the double return at the beginning: ok, let's say that you really need to do this check. As it may be seen as a guard, it may be ok to write a inline return. However, as a rule of thumb, it's better to write full indented conditional expressions, they are much more clear (at the meager cost of a couple of lines and an indentation level).

• #{ match } -> #{match}. Nobody puts those spaces.

• regex = /(\S+\.(com|net|org|edu|gov)(\/\S+)?)/: This looks like a brittle regular expression to detect URLs. Also are you sure you won't get a http:// here? Anyway, you'll know what you need...

• new_string.gsub!: No need to do an in-place update. Use String#gsub with a block instead.

So I'd simply write:

def autolink_string(string)
  regexp = /(\S+\.(com|net|org|edu|gov)(\/\S+)?)/
  string.gsub(regexp) { |url| "<a href='http://#{url}'>#{url}</a>" }
end

Answer 2

You can use gsub! and block directly with a clone.

def autolink_string str
  unless str.empty?
    regex = /(\S+\.(com|net|org|edu|gov)(\/\S+)?)/
    str.clone.gsub!(regex) do |match|
      "<a href='http://#{ match }'>#{ match }</a>"
    end || str # Return str if no match
  end
end

In console:

> str = "this is a test abc.com"
> result = autolink_string str
=> "this is a test <a href='http://abc.com'>abc.com</a>"
> str
=> "this is a test abc.com"
> result
=> "this is a test <a href='http://abc.com'>abc.com</a>"

> autolink_string "no url here"
=> "no url here"

> autolink_string ""
=> ""

> autolink_string "this is a test abc.com and test twitter.com"
=> "this is a test <a href='http://abc.com'>abc.com</a> and test <a href='http://twitter.com'>twitter.com</a>"