Structuring code that shells out to an external script

Question

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

• Mark Thomas: It's actually fairly well-written code. The self. prefixes are not necessary, but that's a preference. Not sure why sync_out is there as basically an alias for parse.

Answer 1

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

Answer 2

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

Answer 3

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