Something like this would do I reckon - not necessarily very extensible if you need some real transformations done to future browse agent names.

A few additional points:

* it would be a lot cleaner to use if/elsif/elsif/.../end, rather than nesting such similar if statements
* return statements are almost always unnecessary in ruby
* predicate conditionals are a common idiom in Ruby and quite readable when you get used to it
* you can skip "else" statements when the conditions are mutually-exclusive

Never. *Ever*, detect the user agent and act differently depending on the agent, unless you really understand why you shouldn't =)

@Nicolás - I don't think the detect method does what you think it does. You can do the following, but I decided I liked the other form better.

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())

Nicolás' first solution is the best (you should have posted two solutions, as the second wouldn't have received a 5-star rating from me...), as it doesn't repeat a single concept; e.g. you want to add a new UA? just add it in the list of UAs! (notice how easy is to externalize it).

The only issue I find in this, is that I don't know if the original author intended to always respect the convention which says that the return identifier string should be the UA's name with non-word chars stripped (he may decide to call Safari "Apple's browser" for all that we know). So my solution is just Nicolás', but using a Hash instead of an Array.

All that being said, NEVER TRY TO DETECT THE USER AGENT, that's BAAAAAAAAAAAADD (tm).

I submitted this earlier, but it didn't get approved for some reason - attempt #2. Naturally, this code presumes all future agent strings will follow the same pattern. But it's too short to not post.

Here's a more thorough version with a simple test

Whoops - shorter regexp. I even re-read the dang section in Pickaxe looking for \W and read right past it.

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.

@chrismo: check that your code would break with the tests I'm posting here, I think they show the original desired behaviour

Some changes to foca's idea:
1) make it more testable
2) make it more declarative with some variables and a method extraction so that I don't have to remember what \W+ does again (I have an awful memory!!)
3) change the method name

I also added some Rspec tests for the next refactorings so it's easy for people to know what's the desired behaviour and to know they are not breaking anything

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 :)

well, that's what I get for being myopic...

@foca: A method extraction purpose is not only reusing code, which I agree it would be YAGNI, in this particular case, and in most cases, I do it for clarity, to improve readability.

A child can understand what remove_non_word_chars means but it will take longer to understand what \W+ and /[^a-z0-9]/+ means, even if you are an expert in regexps. It gives more importance to the purpose and the thought process that occurred when writing this code than the implementation (using regexps).

The method is more testable because I neither need to set the request nor I need to mock it. It's just a string manipulation without a dependency to a request.

The m flag had no sense to be there, I was doing some experiments before posting and I forgot to remove it, but I removed it one minute after posting the code!!!! how did you see it?

@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)

i like what i see here, but there's no instructions for using these methods in a view. in my views, i'd like to do:
<% if browser(ff3) %>hurrah!<% elsif browser(ie6) %>bah!<% end %> can someone refactor the following to make this possible?

here it is. to test firefox, use:

- if browser?(:ff)

in related refactorings, see http://refactormycode.com/codes/535-browser-detection-and-url-redirects-from-array