1
\$\begingroup\$

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
  def initialize(host, user=Etc.getlogin, *args)
    @host = host
    @user = user
    @keys = args.empty?  \
    ? args << Etc.getpwnam(Etc.getlogin).dir + '/.ssh/id_rsa' \
    : args
  end

  ##
  # Download a file over SCP.
  #
  # ==== Attributes
  #
  # * +hostname+ - Target hostname to download the file from.
  # * +filename+ - Filename to download.
  def download_file(remote_path, local_path)
    ssh = Net::SSH.start(@host, @user, :keys => @keys)
    ssh.scp.download(remote_path, local_path, :preserve => true)
    ssh.close
  end

end
\$\endgroup\$
2
\$\begingroup\$

Some comments:

  • 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
    @user = options[:user] || Etc.getlogin
    id_rsa_path = File.join(Etc.getpwnam(@user).dir, ".ssh/id_rsa")
    @keys = options[:keys].empty? ? [id_rsa_path] : options[:keys]
  end

  def download_file(remote_path, local_path)
    ssh = Net::SSH.start(@host, @user, :keys => @keys)
    ssh.scp.download(remote_path, local_path, :preserve => true)
    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].

\$\endgroup\$
  • \$\begingroup\$ This also is great feedback. Thank you. I plan to take items from both the 1st, and 2nd review here. \$\endgroup\$ – fr00z1 Oct 10 '14 at 21:12
  • \$\begingroup\$ +1 for mentioning the updating of arguments. I hadn't noticed, but of course it has side-effects \$\endgroup\$ – Flambino Oct 10 '14 at 21:15
  • \$\begingroup\$ 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! \$\endgroup\$ – fr00z1 Oct 10 '14 at 21:18
  • \$\begingroup\$ Remarking this as the answer as I use more code from this response. Thank you again! Take care. \$\endgroup\$ – fr00z1 Oct 10 '14 at 21:22
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – Flambino Oct 10 '14 at 21:31
2
\$\begingroup\$

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?
      Etc.getpwnam(Etc.getlogin).dir + '/.ssh/id_rsa'
    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
      def initialize(host, user=Etc.getlogin, *args)
        @host = host
        @user = user
        @keys = if args.empty?
          args << Etc.getpwnam(Etc.getlogin).dir + '/.ssh/id_rsa'
        else
          args
        end
      end
    
      def download_file(remote_path, local_path)
        options = {
          ssh: { keys: @keys },
          preserve: true
        } 
        Net::SCP.download!(@host, @user, remote_path, local_path, options)
      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
        Etc.getlogin
      end
    
      def download!(host, remote_file, local_file)
        options = {
          ssh: { keys: [default_key] }
          preserve: true
        }
        Net::SCP.download!(host, default_user, remote_file, local_file, options)
      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.

\$\endgroup\$
  • \$\begingroup\$ Crud.. I forgot to change the documentation. I completely changed the method, so I will fix that. \$\endgroup\$ – fr00z1 Oct 10 '14 at 21:09
  • \$\begingroup\$ Also.. yes.. the goal is to download multiple files from a single host. :-) Thank you so much for the review. \$\endgroup\$ – fr00z1 Oct 10 '14 at 21:11
  • \$\begingroup\$ 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! \$\endgroup\$ – fr00z1 Oct 10 '14 at 21:22
  • \$\begingroup\$ @fr00z No prob; I upvoted tokland's answer myself :) \$\endgroup\$ – Flambino Oct 10 '14 at 21:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.