I am looking for a code review on this class. I found myself writing this logic a lot, so trying to save myself some time.

require 'etc'
require 'net/ssh'
require 'net/scp'

class SSHOps
@host = host
@user = user
@keys = args.empty?  \
? args << Etc.getpwnam(Etc.getlogin).dir + '/.ssh/id_rsa' \
: args
end

##
#
# ==== Attributes
#
ssh = Net::SSH.start(@host, @user, :keys => @keys)
ssh.close
end

end


• def initialize(host, user=Etc.getlogin, *args). Ugly signature. I'd write def initialize(host, options = {})
• I had never seen that multiline way of writing a conditional. It does not look too good...
• Use File::join instead of joining paths by hand.
• The arguments in the documentation are wrong.
• args << Etc.getpwnam(Etc.getlogin).dir + '/.ssh/id_rsa'. It's not considered good practice to update an argument.

I'd write:

require 'etc'
require 'net/ssh'
require 'net/scp'

class SSHOps
def initialize(host, options = {})
@host = host
id_rsa_path = File.join(Etc.getpwnam(@user).dir, ".ssh/id_rsa")
@keys = options[:keys].empty? ? [id_rsa_path] : options[:keys]
end

ssh = Net::SSH.start(@host, @user, :keys => @keys)
ssh.close
end
end


Note that the library active_support has a nice abstraction for the pattern @keys = options[:keys].empty? ? [id_rsa_path] : options[:keys]: @keys = options[:keys].presence || [id_rsa_path].

• This also is great feedback. Thank you. I plan to take items from both the 1st, and 2nd review here. – fr00z1 Oct 10 '14 at 21:12
• +1 for mentioning the updating of arguments. I hadn't noticed, but of course it has side-effects – Flambino Oct 10 '14 at 21:15
• QQ.. what is the rule of thumb of when to use options = {} vs. **kwargs, Your approach is definitely cleaner than my first one, but would definitely appreciate your guidance on this one. Thank you! – fr00z1 Oct 10 '14 at 21:18
• Remarking this as the answer as I use more code from this response. Thank you again! Take care. – fr00z1 Oct 10 '14 at 21:22
• @fr00z My 2 cents: When a lot of your arguments are optional, they're, well, options! :) Besides, in your case, it'd be hard to leave out the user arg, but still pass the keys. You'd have to duplicate the logic in the method just to set user to the default value. If your default was nil it'd be a little easier, but still not great: new(host, nil, *keys). And mixing optional args and a * splat makes the signature even more cumbersome and potentially confusing. – Flambino Oct 10 '14 at 21:31

Some notes:

• Your documentation doesn't match... well, it doesn't match anything in your code! Wrong documentation is the only thing worse than no documentation.

• A ternary should not span multiple lines. Thankfully, a regular if..else is also an expression in Ruby.

@keys = if args.empty?
else
args
end


While you're at it, don't name the argument args: Name it keys, since that's what it is.

• I haven't played around with Net::SCP, but if I'm reading the docs right, download will return immediately (i.e. before the file's been downloaded). And your next line closes the connection. As far as I can guess, you, in effect, just cancel all of your downloads as soon as you start them. What you want is probably download! (note the bang).

• Incidentally, it's perhaps overkill to make a class for this. Net::SCP already has a download! class method. If we rewrite your class with this, we get:

class SSHOps
@host = host
@user = user
@keys = if args.empty?
else
args
end
end

options = {
ssh: { keys: @keys },
preserve: true
}
end
end


At this point, the class is only adding defaults for username and keys. That's it. So it might be simpler to write a few simple helper methods:

module SSHOps
module_function

def default_key
File.join(Etc.getpwnam(default_user).dir, ".ssh", "id_rsa")
end

def default_user
end

options = {
ssh: { keys: [default_key] }
preserve: true
}
end
end


Which you can then use like:

SSHOps.download!("example.com", "/some/remote/file", "/some/local/file")


I've intentionally left out some stuff. My hunch is that you almost always want to use your current username and keys, because that's the functionality that your current class adds. Anything else can basically be achieved just as simply with plain Net::SCP.download!, possibly using the two other helper methods in the module above.

• Of course, if you plan to download multiple files in a row, then it'd make sense to use a class if you at least reuse your SSH session rather than opening one for each download.

• Crud.. I forgot to change the documentation. I completely changed the method, so I will fix that. – fr00z1 Oct 10 '14 at 21:09
• Also.. yes.. the goal is to download multiple files from a single host. :-) Thank you so much for the review. – fr00z1 Oct 10 '14 at 21:11
• This would have definitely been the answer if I were using a module, and all your points were valid. Wish I could mark this as an answer as well. I use most of the answer below, but still took tidbits from your review. I appreciate the time you spent on this. Thank you again! – fr00z1 Oct 10 '14 at 21:22
• @fr00z No prob; I upvoted tokland's answer myself :) – Flambino Oct 10 '14 at 21:33