Modifying an image URL based on hash options

Question

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.

• Mark Thomas: What is raw_info['picture']? An object or just a plain array?
• Ashitaka: raw_info is a hash and raw_info['picture'] is just a string.

Answer 1

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.

• Ashitaka: 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']?
• Ashitaka: Also, what do you think of if condition ... else ... end and return if condition ... else_branch? Which do you prefer?

Answer 2

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