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

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.