1
\$\begingroup\$

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
\$\endgroup\$
2
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ +1 for pointing out that initialize should be doing what it's in start. But -1 for those terribly unidiomatic spaces around parentheses. \$\endgroup\$ – tokland Jun 6 '13 at 19:21
  • \$\begingroup\$ 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. \$\endgroup\$ – m_x Jun 7 '13 at 7:11
1
\$\begingroup\$

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
    
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.