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?
end
return image
end

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

filename = @context.parameters[:cache]
location = 'dual/' + filname
type = 'image/jpeg'
end

def cache_update_request?
@context.parameters.has_key?(:cache)
end

end


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

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

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?
end
return image
end


Could be this:

def start(params)
@context = Context.new params
image = create_image
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)
filename = @context.parameters[:cache]
location = 'dual/' + filname
type = 'image/jpeg'

def upload(image)

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.