Structuring code that shells out to an external script

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 :-).

The code is good already: short, clear methods. I made a few small changes, mostly around naming:

1. Rather than put local and remote into a single array and refer to them by index, just give them both names. It makes client code more readable.
2. Raising an error rather than an exeception, as someone else noted.

Finally, the method names seem slightly opaque. Since I don't unserstand exactly what your doing, I didn't attempt to fix this, but it appears "parse" would be the public method used by clients, and that name is vague. What is the action really accomplishing? I'd pick the name based on that. It seems you are syncing with s3, and then returning the results of that sync. Maybe "sync_results" would be appropriate?

Depending on how you intend clients to use it, you may want to expose sync (with no output, but storing it's raw results in the object) and sync_results separately? Parsing is really an implementation detail that has little to do with the intent of the method (I think) and so should not be the name of the method.

module CompanyLib
  class S3

    attr_reader :output, :local, :remote, :bucket

    def set_path local, remote
      @local = local
      @remote = remote
      @bucket = get_bucket(remote)
    end

    def get_bucket(str)
      /(^[^\/]*)/.match(str)[0]
    end

    def sync
      @output = `aws s3 sync #{local} s3://#{remote} --acl public-read`
      raise "S3 sync command failed: #{output}" unless $?.success?
    end

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

  end
end

It looks a bit of OOP bloat there. Personally I don't like a class with getters and setters working with side-effects all over the place. I don't see why not write a simple function. To add some notes to the other answers:

• @path = [local, remote]: This is bad practice, as you see later when those fields are accessed with @path[0] or @path[1]. That's not declarative.
• I don't get all the normalizing stuff, looks pretty convoluted.

I'd write:

module CompanyLib
  class S3
    def self.sync(local, remote)
      bucket = remote.split("/").first
      output = system("aws s3 sync #{local} s3://#{remote} --acl public-read")
      if $?.success?
        output.split(/\n/).select { |line| line.starts_with?("s3://#{bucket}/") }
      else
        fail("S3 sync command failed: #{output}")
      end
    end
  end
end