current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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
share|improve this question

3 Answers 3

up vote 3 down vote accepted

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.

share|improve this answer
    
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 yesterday
    
Would you please mind explaining the shift the user_locale part? – Saim yesterday
    
@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. – Flambino yesterday
    
Got it, thanks again. In this case, I think, returning nil from user_locale would make more sense and keeps it simple. Agree? – Saim yesterday
    
@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. – Flambino yesterday
up vote 2 down vote

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
share|improve this answer
    
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! – Saim 17 hours ago
up vote 1 down vote

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

share|improve this answer
    
Yes, this looks great. Thanks Borsunho. – Saim 23 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.