foca

[Ruby] On Code to detect the web browser

2008-08-29T17:42:05-07:00

@dcadenas: re: testability: oh, sorry, just saw how you passed in the request.user_agent as a default parameter, missed that on my first read =) And yeah, I agree that's more testable.

re: undestandability: I suppose it's a matter of how you look at it. If the *purpose* of the method is to ensure that all non-word characters are removed from the ua, then it might make sense to extract that to a method. If it's just a means to an end (getting an "identifier" for the UA), then adding a method will convolute things (you will see that and think that it's important because of the behavior, when it's just a consequence...)

In any case, returning a symbol would make more sense for this particular use case, instead of the different strings, so this might be a non issue after all (and then the best solution would be using the hash as nachokb proposed)

[Ruby] On Code to detect the web browser

2008-08-29T02:23:50-07:00

Why is that more testable?

I don't think extracting the gsub call into its own method makes any sense. If you have trouble remembering what \W means, either a) print a regexp cheatsheet or b) use /[^a-z0-9]/+

And BTW, you use the "m" flag, which I find even harder to remember than \w =P -- if it's the one that makes "." take into account linebreaks (and thus \W and all char classes, too), then it doesn't apply here anyway, since the user agent can't have linebreaks.

And if the answer is "because you'll use it later" in a string that needs to replace a \n then I say YAGNI. Don't refactor code that's not there yet. Don't over engineer it :)

[Ruby] On Code to detect the web browser

2008-08-29T01:51:29-07:00

Hm, good one. But what nachokb pointed out above, with his refactoring my array for a hash of "translations" makes sense tho.
Quick regexp thing: if you can't use \W for some reason (maybe periods and commas are allowed), you can still shorten your regexp by using /[a-z0-9]/i where the 'i' flag means case insensitive.

[Ruby] On Code to detect the web browser

2008-08-28T22:07:41-07:00

Er, sorry, I rushed to post and messed up the code, the idea was the one following.
(Also changing the array, it's much more clear to use a straightforward array than using %w())

def test_web_browser
  ["Firefox/3", "Firefox/2", "MSIE 6", "MSIE 7", "Opera"].detect do |agent|
    request.user_agent =~ Regexp.new(agent)
  end.gsub(/\W+/, "")
end