HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Structuring code that shells out to an external script

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scriptstructuringshellsthatcodeexternalout

Problem

This code that shells out to an external script could use some input in order to read smoother, I think. Praise and critique are welcome.

module CompanyLib
  class S3

    attr_reader :output

    def set_path local, remote
      @path = [local, remote]
    end

    def get_bucket
      /(^[^\/]*)/.match(@path[1])[0]
    end

    def sync
      @output = `aws s3 sync #{@path[0]} s3://#{@path[1]} --acl public-read`
      raise Exception, "S3 sync command failed: #{output}" unless $?.success?
    end

    def parse
      self.sync
      normalize = output.gsub("\n", "\\n")
      normalize.scan(/s3:\/\/#{self.get_bucket}(.*?)\\n/).flatten
    end

    def sync_out
      self.parse
    end
  end
end

Solution

Overall looks like a good start, some thoughts I had:

-
If I were implementing this class I probably wouldn't just store the output as a string, I'd parse it in a way that callers would be able to use it effectively. For example, does it output total time, data transferred, files copied, etc.? If you're going to try to use this data, I think this class (or a class used by this class) should have the responsibility to process that data, not the caller.

-
Some prefer the system() method over in-line backticks, for readability, but that's preference.

-
Never raise Exception, either raise RuntimeError by omitting the first parameter to raise or fail, or create a new exception class (with RuntimeError as a parent) for your purpose. Similarly you should (in almost all cases) never rescue Exception.

-
In one of the codebases I work on were have an entire class dedicated to executing commands locally. One reason we do this is that you can't isolate stdout from stderror using backticks or system(), so we use the popen4 library and generate a Result class that has the exit code, stdout and stderr. The stderr may be useful if you're trying to relay to the caller what failed exactly. The caller may want to retry a timeout, but not an authentication issue, for example.

I don't know how widely used or important this class is for your company, so feel free to file any of these under "over-engineering" as appropriate :-).

Context

StackExchange Code Review Q#63077, answer score: 2

Revisions (0)

No revisions yet.