Image uploader and manager

Question

I have recently started using Ruby and would like to hear your take on the following piece of code.

class Generator
 def initialize
 @context = nil
 end
 def start(params)
 @context = Context.new params
 image = create_image
 if cache_update_request?
 upload image
 end
 return image
 end
 def create_image
 composer = Composer.new @context
 execution_context = ExecutionContext.new(@context, composer)
 execution_context.render
 end
 def upload(image)
 uploader = CompositorCommons::CacheUpdater.new
 filename = @context.parameters[:cache]
 location = 'dual/' + filname
 type = 'image/jpeg'
 uploader.upload(image,location,type)
 end
 def cache_update_request?
 @context.parameters.has_key?(:cache)
 end
end

Answer 1

Your code is nice overall, but here are my two cents :

• initializing instance variables before using them is not mandatory, so initialize as you implemented it is useless. A call to @context if it is not initialized will return nil.

• Since uninitialized instance variables return nil, you may have have problem when doing things like @context.parameters[:cache]. As such, i would advise to initialize them... in initialize :

  class Generator
   def initialize( params )
   @context = Context.new( params )
   end
  end
  

This also leaves you the latitude to inspect the params and throw an ArgumentError if some usefull params are not present.

• I think your object has mixed responsibilities. What does it represent ? It is not clear at first glance. Is it an uploader, an image ? It looks like a Service to me (the process is stateless), or maybe a "context" in DCI slang. Maybe your structure could more accurately represent this :

  module ImageGeneratorService
   def self.start( params )
   cache = params.delete( :cache )
   image = ImageFactory.create_from_params( params )
   CacheService.cache_image( image, cache ) if cache
   image
   end
  end
  

... ok, it's a bit too much, but i think you get the spirit. YMMV

• tokland: +1 for pointing out that initialize should be doing what it's in start. But -1 for those terribly unidiomatic spaces around parentheses.
• m_x: I used not to use parentheses at all... But then, there a are times you have to use them, and when you do it (when chaining ActiveRecord query methods, for instance) in my experience it is much more readable to add spaces, because arguments stand out. Then again, it's just a matter of taste... I usually adapt to whatever conventions my team use.

Answer 2

Looks OK, except for a few small notes on style.

• Don't return, just let things fall off the end of methods:

  This...

  def start(params)
   @context = Context.new params
   image = create_image
   if cache_update_request?
   upload image
   end
   return image
  end
  

  Could be this:

  def start(params)
   @context = Context.new params
   image = create_image
   upload image if cache_update_request?
   image
  end
  

• Drop some of the temporary variables

  While I'm not generally a fan of excessively long one-liners, I think several of your multiline methods could be written using a fraction of the characters as one-liners:

  This...

  def create_image
   composer = Composer.new @context
   execution_context = ExecutionContext.new(@context, composer)
   execution_context.render
  end
  

  Could be this:

  def create_image
   ExecutionContext.new(@context, Composer.new @context).render
  end
  

  This...

  def upload(image)
   uploader = CompositorCommons::CacheUpdater.new
   filename = @context.parameters[:cache]
   location = 'dual/' + filname
   type = 'image/jpeg'
   uploader.upload(image,location,type)
  end
  

  Coule be this:

  def upload(image)
   uploader = CompositorCommons::CacheUpdater.new
   filename = @context.parameters[:cache]
   uploader.upload(image, "dual/#{filename}", "image/jpeg")
  end
  

Answer 3

It also feels like unless you really want to expose the create_image, upload, and cache_update_request? in a public interface, you should encapsulate them and make them private.