Argument passing to dumb tasks

I'm becoming acquainted with ruby by taking some of my legacy shell scripts and rewriting them as .rb.

The pattern that is emerging is that I have a number of different dumb tasks, that do what they are told, and a coordinator that is responsible for ordering the tasks, and providing to the tasks the data that will be needed.

For instance, at the end of one of my jobs, I will need to convert some data to a human readable format, perform a two phase upload of that data to a remote server, and then send a notification to subscribers announcing the successful delivery of the data.

class PublishReportProcess
  def initialize(convertor, uploader, email)
    @convertor = convertor
    @uploader = uploader
    @email = email
  end

  def run(task = {})
    convert_context = {}
    convert_context[:type] = 'geo'
    convert_context[:output] = "#{task[:workspace]}/#{task[:report_id]}.json"

    @convertor.convert(convert_context)

    upload_context = {}
    upload_context[:local] = convert_context[:output]
    upload_context[:staging] = "#{task['reports']['workspace']['uri']}/#{task['report_id']}"
    upload_context[:remote] = "#{task['reports']['repository']['uri']}/#{task['report_id']}"

    @uploader.upload(upload_context)

    email_context = {}
    email_context[:message] = <<MESSAGE
There is a new report available at #{task['reports']['repository']['uri']}/#{task['report_id']}.

Share and enjoy
MESSAGE

    @email.email(email_context)
  end
end

The implementation of the tasks, if it matters, tend to be using the arguments of the map passed to them to create a command line string that can be passed to Open3.popen2e.

The standard American English spelling is "converter", not "convertor". If it's different where you live, use the appropriate regional spelling, but if you live in America, use the American spelling. Since I do, I did.

You have @converter and @uploader, but @email. I suggest changing it to @emailer, for consistency's sake.

You mix symbols and strings as Hash keys. I suggest using all symbols, since it's much more standard. This means that

task['reports']['workspace']['uri']

becomes

task[:reports][:workspace][:uri]

Is the workspace at [:reports][:workspace] or [:workspace]? Based on the very similar [:reports][:repository] that you use, I'm assuming it's the former. Pick one and stick with it.

You use #{task[:reports][:repository][:uri]}/#{task[:report_id]} a couple times in your strings, and it's long. Why not declare a variable?

repository_uri = "#{task[:reports][:repository][:uri]}/#{task[:report_id]}"

repository_uri might not be the best name; I'd recommend picking whatever works best.

Your heredocs are weird. What I normally see is something like this:

puts  /, '')
        > Stuff!
        > More stuff!
        > #{pound_sign}Octothorpe
      MESSAGE

Note the two differences between that syntax and yours:

• Instead of






• Immediately after the beginning tag, I have .gsub(/\s*> /, ''). This operates exactly like a normal .gsub call on the contents of the heredoc, after string interpolation, and you can feel free to change the regex in front to whatever suits you. It's there to let you specify where the line starts; in effect, it allows you to indent the contents and have indenting in the string itself. Note that this specific regex requires a space after the > or the entire line is there, unchanged. Also, it uses [ \t] rather than \s because \s will also match newlines, and it's probably better to allow multiple lines.




The benefit of this approach is that you don't have to get rid of all indents for however many lines; the drawback is that it's ever so slightly slower, because it has to perform a gsub on the end result.

Now, you build your hashes oddly. Rather than creating an empty one, then putting values in it, why not build it with the values already in it? For example, this:

convert_context = {}
convert_context[:type] = 'geo'
convert_context[:output] = "#{task[:workspace]}/#{task[:report_id]}.json"

becomes this:

convert_context = {
  type:   'geo'
  output: "#{task[:workspace]}/#{task[:report_id]}.json"
}

If you store "#{task[:workspace]}/#{task[:report_id]}.json" as a variable (let's call it workspace_url; you'll want a better name) then you can use this super cool syntax for calling methods with hashes:

@converter.convert(
    type: 'geo',
    output: workspace_url,
)

You could, of course, put the string in directly in both places where you reference it, if you want to. It's your choice.

Note that with heredocs, you have to put the comma on the first line for it to register; otherwise, it doesn't see the end of the doc . This is due to a mildly annoying quirk in how heredocs work.

If I'm using this class and see that the parameter is optional, and I don't know what it does, I'm gonna leave it out, then when it starts putting nothing in the URLs (or throwing NoMethodError: Undefined method []' for nil:NilClass) I'm gonna be really confused, since it should work with the default argument, instead of throwing errors!

Of course, someone could easily supply a blank Hash themselves. It's much safer to create a basic ReportableTask (because Task is really generic and a bad name for a class) class as a data wrapper; something like this is all you need:

class ReportableTask
  def initialize(workspace, reports, report_id)
    @workspace = workspace
    @report_id = report_id
    @reports = reports
  end

  attr_accessor :workspace, :report_id, :reports
end

You could even put in some convenience methods, like repository_uri that returns @reports[:repository][:uri].

Then, if you insist on being able to use your code totally unchanged, you can add a method:

def [](name)
    instance_variable_get("@#{name}")
end

but I'd recommend just changing your code.

Then you could make similar objects for reports -- maybe a Report class? -- and maybe lots of other things. Without seeing the structures you have in mind I can't tell you for sure what'd be best or even give advice on providing that, but in general if you're making lots of hashes with all the same keys it's good to create a class, especially if the usage of those hashes depends on them always having those keys.

Anyway, aside from those tips, it's pretty hard to give a more complete/accurate review without seeing examples of the data you're passing in.

With the changes I suggested (except the one about adding a new class), here's what your code looks like:

```
class PublishReportProcess
def initialize(c

StackExchange Code Review Q#54593, answer score: 7