2
\$\begingroup\$

I'm new to Rails/Ruby and I would like to get feedback on my little ApplicationController which is able to detect the visitor's language. I'm especially interested in optimizations on the control structure in the set_locale method (2 nested if statements and 3 assignments to one variable). I'm using the http_accept_language gem.

class ApplicationController < ActionController::Base
  AVAILABLE_LANGUAGES = %w{de en}

  before_filter :set_locale
  protect_from_forgery


  def set_locale
    if params[:locale].nil?
      if preferred_language = request.preferred_language_from(AVAILABLE_LANGUAGES)
        I18n.locale = preferred_language
      else
        I18n.locale = I18n.default_locale
      end
    else
      I18n.locale = params[:locale]
    end
  end

  def default_url_options(options={})
    logger.debug "default_url_options is passed options: #{options.inspect}\n"
    { :locale => I18n.locale }
  end
end
\$\endgroup\$
  • \$\begingroup\$ Just a note: In Ruby, an empty string is true. Checking for params[:local].empty? is better than nil? since nil? returns false for empty strings, and the locale can be an empty string. \$\endgroup\$ – Mohamad Jan 30 '15 at 17:02
  • \$\begingroup\$ @Mohamad Completely agree with you on a technical basis, but I think you need to reword your first sentence because "" == true => false \$\endgroup\$ – wintersolutions Jan 30 '15 at 17:07
  • \$\begingroup\$ Sorry, I meant to say an empty string would evaluate to true, and not that it equals or returns true. Thanks for pointing that out. For exmaple: {foo: ''}[:foo].nil? => false \$\endgroup\$ – Mohamad Jan 30 '15 at 17:24
1
\$\begingroup\$

You can do it in an "one-liner". Although one-liners are frequently associated with less readability, I honestly believe this is not the case. Something like this would work:

I18n.locale = params[:locale] || 
              request.preferred_language_from(AVAILABLE_LANGUAGES) ||
              I18n.default_locale

I see a few advantages in this solution:

  • Your code will be a bit cleaner, since you can collapse 9 lines of code filled with if/else conditions into 3 (or 1, if you don't break the line);
  • You won't need to allocate preferred_language in case params[:locale] is nil;
  • And the biggest advantage of them all: clarity. The precedence of the options is much more explicit.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This is so simple and great, how could I not come up with it in the firstplace? Thanks! \$\endgroup\$ – wintersolutions Feb 1 '12 at 8:07
1
\$\begingroup\$

I like the one liner from goncalossilva, but I would probably add a check to the first clause.

def valid_locale(locale)
  locale if AVAILABLE_LANGUAGES.include?(locale)
end


I18n.locale = valid_locale(params[:locale]) || 
              request.preferred_language_from(AVAILABLE_LANGUAGES) ||
              I18n.default_locale
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Good catch there, I tought Rails would fallback to the default language if the locale wasn't found, as a quick test showed I was wrong! \$\endgroup\$ – wintersolutions Feb 1 '12 at 12:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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