1
\$\begingroup\$

The code I'm trying to improve modifies a URL according to the options passed in a hash:

{
  :image_aspect_ratio => "square",
  :image_size => 50
}

or

{
  :image_size => {
    :width => 50,
    :height => 60
  }
}

The code looks like this:

module X
  module Y
    class Z


def image_url
  if raw_info['picture'] && image_size_opts_passed?
    image_url_with_size_opts
  else
    raw_info['picture']
  end
end

def image_size_opts_passed?
  !!(options[:image_size] || options[:image_aspect_ratio])
end

def image_url_with_size_opts
  params_index = raw_info['picture'].index('/photo.jpg')
  if params_index
    raw_info['picture'].insert(params_index, image_params)
  else
    raw_info['picture']
  end
end

def image_params
  image_params = []
  if options[:image_size].is_a?(Integer)
    image_params << "s#{options[:image_size]}"
  elsif options[:image_size].is_a?(Hash)
    image_params << "w#{options[:image_size][:width]}" if options[:image_size][:width]
    image_params << "h#{options[:image_size][:height]}" if options[:image_size][:height]
  end
  image_params << 'c' if options[:image_aspect_ratio] == 'square'

  '/' + image_params.join('-')
end

Here's the stuff I hate about this code:

  1. Lots of raw_info['picture'] being called, but I'm not sure if using a local variable is better than accessing a hash twice.

  2. I'm seeing some duplication in the else branch in the image_url and image_url_with_size_opts methods but I don't know how to improve that.

  3. This code is all inside a class which is inside a module which is inside another module, so I'm not sure if I could also memoize the image_params result.

\$\endgroup\$
2
  • \$\begingroup\$ What is raw_info['picture']? An object or just a plain array? \$\endgroup\$ Aug 26, 2013 at 0:38
  • \$\begingroup\$ raw_info is a hash and raw_info['picture'] is just a string. \$\endgroup\$
    – Ashitaka
    Sep 4, 2013 at 15:53

2 Answers 2

2
\$\begingroup\$

Overall, it's reasonably tight code. You've taken steps to reduce method size, which is good.

To answer your questions specifically:

  1. I think an instance variable would be better, because then you can avoid the else clauses you mention in 2. Those methods can then be reduced to 1 or 2 lines.

  2. see 1. :)

  3. If you create an image or picture object, it provides a convenient place to memoize results. I'm not sure this is that important though, as it's probably not likely you'd need to generate more than one URL for a given image in a single request.

\$\endgroup\$
2
  • \$\begingroup\$ I made a mistake in my first question. I used the term "instance variable" when I meant to say "local variable". raw_info is already an instance variable (well, it's a method with a memoized result). And it's a hash. But since I'm accessing the same hash key so many times, should I move it to a local variable? Or should I create a memoized method to hold raw_info['picture']? \$\endgroup\$
    – Ashitaka
    Sep 4, 2013 at 15:51
  • \$\begingroup\$ Also, what do you think of if condition ... else ... end and return if condition ... else_branch? Which do you prefer? \$\endgroup\$
    – Ashitaka
    Sep 4, 2013 at 15:55
0
\$\begingroup\$

You might consider using a case statement for image_params:

    def image_params
            image_params = []
            case options[:image_size]
            when Integer
               ....
            when Hash
               ....
            else
               ....
            end
            .....
    end

You'll recall this works because case uses ===.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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