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?

• tokland: This method needs some polishing indeed, but are you aware that there are gems for this task? github.com/tenderlove/rails_autolink
• B Seven: Yes, I did some research. This is not a rails project, and from what I could guess rails_autolink is only for Rails. I didn't find any other useful gems. It turns out that the regex is simple anyway.
• tokland: Ok. Let's see if somebody else answers (I am kind of hoarding the ruby tag lately ;-))
• Flambino: I don't know what kind of input you're dealing with, but here are some things to consider: What if the URL already includes "http:" (or something else)? What if there are "bad" chars after the domain, e.g. it's at the end of a sentence and followed by a period? What if the domain is a .co.uk, .io, or .museum or any one of the other top-level domains? What about chars that are illegal in URLs? What if the URL is uppercase? Robust URL detection is hard, but here's a pretty good one
• B Seven: @Flambino - I have a helper method to handle the case that it already includes http://. I omitted it for clarity. It does not match the punctuation after the URL. I don't care about .co.uk, .io, etc. I don't care about illegal URL chars. It works for uppercase. Thanks for the link.

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

• Billy Chan: OP should pick this answer, better than mine. gsub is the way to go, I used it before but failed due to something wrong.
• tokland: that's very nice of you :-) but your answer also shows two of my points: it uses real conditionals and gsub with blocks.

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>"

• B Seven: It seems that using gsub in the block is a good idea. However, I don't want to mutate the string.
• Billy Chan: @BSeven, then just clone it, no more lines needed. Results are same.
• B Seven: Works well, except that it returns nil if there are no matches: autolink_string "no url here" => nil
• Billy Chan: @BSeven, nice point! It depends on your usages, sometimes nil is preferred when you already have "str" in hand. If you choose to return something, I've revised the code to return str if no matches.