Yesterday I've done something I should do more often: Revisit some code written a while ago for our current project and make it better.

Let's face it. We all write crappy code the 1st time. The difference is in what we do about it afterwards.
We might decide it's good enough and keep moving, or we could (and should!) stop and refactor it!

The code I revisited worked as a refactoring exercise and it's initial version is shown below:

class Jphoto
 ...

 #a few other methods ...

 def post_photo(file_data, hotel_id, send_rss, options = {})
   file_name = "tmp/#{Time.now.to_i}_#{rand(1000000).to_s(36)}"
   File.open(file_name, "wb") do |f|
     f.puts(file_data)
   end

   params = [Curl::PostField.file('photo',file_name),
      Curl::PostField.content('hotel', hotel_id),
      Curl::PostField.content('source','PhotoUploadTest')]
   extract_extra_params!(params, options)

   c = Curl::Easy.new("#{service_uri_base}/photoupld")
   c.multipart_form_post = true
   c.http_post(*params)

   if c.response_code != 200
     error_msg = "File upload failed with code: #{c.response_code}"
     Rails.logger.info error_msg
     raise error_msg
   end

   File.delete(file_name)

   hotel = Hotel.find_by_id(hotel_id)
   hotel.cache.destroy_all

   send_upload_rss(hotel, original_upload_url(c.body_str) , options) if send_rss
 end

 private

 def send_upload_rss(hotel, photo_url, options)
   ...
 end
 def manage_images_link(hotel_id)
   ...
 end

 def extract_extra_params!(params, options)
   params << Curl::PostField.content('status', options[:status]) if options[:status]
   params << Curl::PostField.content('upload_source', options[:upload_source]) if options[:upload_source]
   params << Curl::PostField.content('uploader_ip', options[:uploader_ip]) if options[:uploader_ip]
   params << Curl::PostField.content('uploader_email', options[:uploader_email]) if options[:uploader_email]
 end
end

Look at the post_photo method. It has problems in so many levels that it’s hard to start.
Methods should do “one thing” and that method obviously does much more than that, mixing different levels of abstraction.

But let’s start with the easy parts first, keeping in mind that I was aiming for readability.

Lines 7 to 10 seem to be there just to make the reader’s life harder. It’s creating a temporary file through some custom logic instead of using the tools provided by the language. Unnecessary and only pollutes our eyes. My first measure was to use ruby’s TempFile class for this. Better, but we still have a long way.

Right at line 12 it creates some sort of default parameters list, after which it extracts some extra options. I don’t know what that method does but it’s clearly using output arguments, which we should avoid at all costs, as they lead to confusion. This is a big smell as well, and another refactoring step added to my list.

On line 21 starts the code that handles what to do when we get a response_code other than 200 from our request. Apart from the fact that this code doesn’t feel right here, we just happen to know that in HTTP, 200 means success, but that might not be clear to someone looking at the code for the 1st time.

Then the code goes on to delete the temp file, clear the hotel’s cache and send the rss if the rss’ flag is true.

Let there be refactoring….

Geez, how many lines have I used to explain what the code does? Since I don’t wanna bore you to death, here is my refactored version of this method, trying to avoid as much as I can the problems I highlighted previously: