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:
Lots of
raw_info['picture']being called, but I'm not sure if using a local variable is better than accessing a hash twice.I'm seeing some duplication in the else branch in the
image_urlandimage_url_with_size_optsmethods but I don't know how to improve that.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_paramsresult.
raw_info['picture']? An object or just a plain array? \$\endgroup\$raw_infois a hash andraw_info['picture']is just a string. \$\endgroup\$