Setting locale based on preferences with fallbacks

Question

Consider the following code. Are the multiple assignments to the loc variable a code smell? If so, how can I refactor this?

def set_locale
 begin
 loc = user_signed_in? ? user_locale : browser_locale
 loc = browser_locale if loc.nil?
 loc = param_locale if loc.nil?
 loc = I18n.available_locales.include?(loc.to_sym) ? loc.to_sym : I18n.default_locale
 rescue
 loc = I18n.default_locale
 ensure
 cookies.permanent[:my_locale] = loc
 end
 I18n.locale = loc
end

Answer 1

I'd say it's a code smell. Mostly because loc seems to change type. It's one thing for it to be nil or a symbol; in that case it might "only" change value. But the last to_sym would seem to suggest it could also be a string or something.

In terms of code style, you could just use the ||= assignment operator instead of x = y if x.nil?. That's the Ruby way to provide a fallback/default value in case of nil.

However, you're looking to prioritize locale selection, in which case I'd do something like:

locale = [user_locale, browser_locale, param_locale]
 .compact
 .map(&:to_sym)
 .detect { |locale| I18n.available_locales.include?(locale) }
I18n.locale = locale || I18n.default_local
cookies.permanent[:my_locale] = I18n.locale

It'll go through the list and return the first locale that's not nil and is available. If nothing's returned, it uses the default locale.

Note that I'm making an assumption here, namely that user_locale will return nil if the user isn't signed in. But you could also simply unshift the user_locale onto the array if the user is signed in, before doing the detect.

On another note: param_locale should, I think, be the first choice in all cases. If I go to a URL that has an explicit locale code in it, I'd expect to see exactly that page, in that language. URLs should be a permanent as you can make them. Sure, give me an option to view the same URL with a different locale, but don't second guess the URL itself.

• Saim: Perfect. That looks awesome! I was thinking the array.compact thing as well but the its the detect part I unable to think of. As to param_locale, I totally agree with you that it should be the first option. How can I give you +1*100? ;)
• Saim: Would you please mind explaining the shift the user_locale part?
• Flambino: @Saim I actually meant to write unshift - not shift. I just meant that you could build the basic array (i.e. browser & param locales), and only add the user's locale (via unshift, if it has to go first) if the user is indeed logged in, and then do the compact/detect. It's just if you do need to check whether the user's logged in before you called user_locale. But if user_locale just returns nil for non-logged-in users, then you don't need to worry about it.
• Saim: Got it, thanks again. In this case, I think, returning nil from user_locale would make more sense and keeps it simple. Agree?
• Flambino: @Saim Yeah, I'd say so. Getting nil when asking for the user's locale without a user seems reasonable. The alternative would be raising an exception or something, which just seems like it'd complicate everything. Rather just have the method return a locale or nil. You have other ways to check if there is a logged-in user anyway.

Answer 2

Some notes on your code:

• Catch-all rescues look suspicious. At least, you should only catch exceptions that inherit from StandardError. For that, you write rescue => exc. Anyway, what are you catching exactly here?

• loc has many different values, not good practice. Different values, different names.

• To save 3 characters you end up with a ambiguous variable name (locale? location?). It's not worth it IMO.

• Use ||.

I'd write:

def set_locale
 locale = (user_locale || browser_locale || param_locale).to_sym
 I18n.locale = locale.in?(I18n.available_locales) ? locale : I18n.default_locale
 cookies.permanent[:my_locale] = I18n.locale
end

• Saim: Yes, I agree with you on Catch-all thing. Your set_locale version looks slick. Thanks I've learned a lot in even one code review. Thank you guys!

Answer 3

@Flambinos answer is great and covers many important things, but I really think this task should be split in 2-3 methods (depending if you could use list of this locale symbols anywhere else):

def symbolic_locales
 [user_locale, browser_locale, param_locale].compact.map(&:to_sym)
end
def proper_locale
 (symbolic_locales & I18n.available_locales).first || I18n.default_locale
end
def set_locale_to_proper
 cookies.permanent[:my_locale] = I18n.locale = proper_locale
end

This would allow to (削除) play with (削除ここまで) refactor code easier.

Also, shorter methods tend to read better, because amount of natural documentation is higher.