7
\$\begingroup\$

I have written this Ruby script to download some file from server using SFTP. Help me improve this one. Find the original script at this link

#This script is used to download file from the server to your local machine
#The implementation is based on pure Ruby implementation of SFTP using gem Net SFTP
#To install this on your local machine please run the following command in your terminal 'gem install net-sftp'
#Author : Ashish Wadekar <ashishawadekar@gmail.com>
#Date : 18 December 2016

#This is the gem / library required for the SFTP connection management
require 'net/sftp'

#The host address
HOST = "www.example.com"

#User for connection
USER = "admin"

#The location of the authentication key in pem format
SERVER_KEY_LOC = "/location/of/serverauthenticationkey.pem"

#The location on server where DB backups are done
SERVER_BACKUP = "/location/of/file/on/server"

#The location on local machine where the db has to be downloaded
LOCAL_BACKUP = "/location/on/local/machine/where/file/needs/to/be/downloaded"

#Array to hold the file names for easier sorting
files = Array.new

#Array to hold the file details for easier sorting
files_details = Array.new

#Inititalising the connection
Net::SFTP.start(HOST, USER, :keys => [SERVER_KEY_LOC]) do |sftp|
  puts "Connection to Server successful"
  entries = sftp.dir.entries([SERVER_BACKUP]).sort_by(&:name) #Sorting the files by name as timestamp is used as filename
  entries.each do |entry|
    files << entry.name
    files_details << entry.longname
  end

  download_size = files_details[-1].split(' ')[4].to_i

  puts "Total #{(files.size.to_i) - 2} backups found on server."
  puts "The latest file is : #{files[-1]}, Size : #{(download_size)/1048576} MB"
  puts "Now donwloading the file to your local machine at path : #{LOCAL_BACKUP}"


  sftp.download!(SERVER_BACKUP+files[-1], LOCAL_BACKUP+files[-1]) do |event, downloader, *args|
    case event
    when :open then
      # args[0] : file metadata
      puts "Starting download from server: #{args[0].remote} -> #{args[0].local}"
      puts "Perhaps you can sip a coffee till then!"
    when :get then
      # args[0] : file metadata
      # args[1] : byte offset in remote file
      # args[2] : data that was received
      STDOUT.write "Downloading complete: #{args[1].to_i/1048576}MB, #{args[1].to_i}bytes of #{download_size} bytes, #{((args[1].to_i / download_size) * 100.0)}% \r" unless args[1].to_i == 0

    when :close then
      # args[0] : file metadata
      puts "Done! Closing connection to server."
    when :mkdir then
      # args[0] : local path name
      puts "Creating directory #{args[0]}"
    when :finish then
      puts "Backup download complete!"
    end
  end
end
\$\endgroup\$
6
\$\begingroup\$

One of the core elements your code is missing is object-orientation. Take advantage of Ruby's powerful object model. That alone will vastly improve what you have.

What we're going to do is progressively breakdown everything into chunks of logic and encapsulate these chunks of logic in objects and methods.

Before that, let's go through each line and give feedback if anything seems a little off..


#The host address
HOST = "www.example.com"

It seems you've defined quite a few constants for things which, by nature, will change on a user-by-user basis. They are not constants, they are variables, and so it would be better to use variables.

However, if you do replace them with variables, you will not see any real benefit because the code is largely a procedural script. You have to define the constants/variables directly in the script and load that. You will see the benefit of using variables instead once the arguments and execution of the script are separated.

#Array to hold the file names for easier sorting
files = Array.new

You almost never need to initialize variables like this in Ruby. Look into inject for an alternative.

entries.each do |entry|
  files << entry.name
  files_details << entry.longname
end

This, along with the empty collections you defined above, give the impression that something is wrong. We are storing files and file details in separate arrays. There appears to be a focus on primitives, where we should instead be thinking about how we can encapsulate these in objects. There is nothing wrong with using arrays, but we should think about another approach when it becomes obvious that simple primitives are not providing the functionality we need in a clean way.

download_size = files_details[-1].split(' ')[4].to_i

The drawback of a simple array is made immediately obvious on the above line.

puts "Total #{(files.size.to_i) - 2} backups found on server."

We have littered about puts statements as a way to log the execution of the script. Perhaps we later want to log this to a file instead? That would be a good enough reason to extract it into its own object, and, if we want, later change the method of how we log.

puts "The latest file is : #{files[-1]}, Size : #{(download_size)/1048576} MB"

Another common mistake, though easily fixed, is the magic number 1048576. Give it some kind of meaning by hiding it behind a descriptive variable/constant or method.

# args[0] : file metadata

Along with magic numbers, we have magic indexes being used everywhere. You yourself can see this is difficult to follow and so have created a comment to remind you what you are dealing with. Again, this should be hidden behind a descriptive variable or method.

STDOUT.write "..."

We have now changed from puts to STDOUT.write. I haven't played around with this so unsure as to why, but lets assume this something we can also include in our logging object.

On a side-note - we have lots of comments in this code. As you begin to hide details behind descriptive names, you will see that the value in comments begins to diminish. If you have to litter comments everywhere then perhaps your code isn't readable enough?


Now let's start refactoring. With all that in mind, where do we begin? I can show you the end result, but that isn't useful in learning how to approach this in the future. The focus on creating small, composable objects and methods should be an ever-present design decision when programming Ruby. We are learning to think in objects.

The first thing I would do is throw the entire thing into a single object. Something like:

class RubySFTP
  HOST = "www.example.com"
  # etc

  def start
    files = Array.new
    files_details = Array.new

    Net::SFTP.start #etc
  end
end

This has immediately brought up the concept of RubySFTP. We would use as so: RubySFTP.new.start. It should be noted I could have created a class-method self.start and simply called RubySFTP.start, but that would take away the need to instantiate the object, which we will quickly make use of as we begin to breakdown our logic.

Again, focusing on small, descriptive methods, we can see start does not satisfy this at all. Because the logic is wrapped in a block, it is difficult to modify this to our tastes. For that reason, we will get rid of the block and simply use Net::SFTP.start(HOST, USER, :keys => [SERVER_KEY_LOC]).

That means we would then have:

  def start
    files = Array.new
    files_details = Array.new

    sftp = Net::SFTP.start # etc

    sftp.dir.entries # etc
  end

We are now initializing 3 variables in a single method. This can indicate a kind of code-smell when working with objects. We could clean this up greatly by extracting them into methods of the same name. When we do this it is important we don't reinitialize them every time they are called, so we use memoization.

For example, instead of sftp = [..], we have:

def sftp
  @sftp ||= Net::SFTP.start(HOST, USER, :keys => [SERVER_KEY_LOC])
end

If we do that with those other two variables (files and files_details), our start method now looks like:

def start
  sftp.dir.entries #etc
end

Now it is simply a case of running through things and seeing what we can extract into other methods..

entries = sftp.dir.entries([SERVER_BACKUP]).sort_by(&:name)

..becomes..

def sorted_entries
  @sorted_entries ||= sftp.dir.entries([SERVER_BACKUP]).sort_by(&:name)
end

..

download_size = files_details[-1].split(' ')[4].to_i

..becomes..

def download_size
  @download_size ||= files_details[-1].split(' ')[4].to_i
end

..and so on.. One other note here - though I am using memoization for each of these methods, this is only really a good idea if you are calling that method more than once. We might remove that later on, but noticing where you could benefit from memoization is a good habit to develop.

If we remove all the puts for now, let's see how this has affected the start method..

def start
  sorted_entries.each do |entry|
    files << entry.name
    files_details << entry.longname
  end

  sftp.download!(SERVER_BACKUP+files[-1], LOCAL_BACKUP+files[-1]) do |event, downloader, *args|
    # etc
  end
end

As you can see, this has clearly cleaned things up. We are hiding implementation details behind descriptive methods which is allowing us to think about the program from a better perspective.

Next, the way we are building files and files_details and the way we are accessing them is not that great - files_details[-1].split(' ')[4].to_i. We would much prefer if we could do something like file.download_size instead.

Ideally, what we want to do is wrap the files in our own object so we can build a more useful interface. We could also wrap the collection of files, but for simplicity we still use an array.

That means this:

entries.each do |entry|
  files << entry.name
  files_details << entry.longname
end

Should be, for one, in its own method, and, two, wrap the files (entry). That would look like:

def entries
  sorted_entries.map { |entry| Entry.new(entry) }
end

We would then build an Entry class and define a simple interface to extract what we're looking for. For example:

class Entry
  def initialize(entry)
    @entry = entry
  end

  def name
    @entry.name
  end

  def download_size
    @entry.longname.split(' ')[4].to_i
  end
end

Now our start method simply has:

def start
  sftp.download!(SERVER_BACKUP+entries[-1].name, LOCAL_BACKUP+entries[-1].name) do |event, downloader, *args|
    # etc
  end
end

The next thing we want to remove is those magic indexes (entries[-1]). We could do that by defining a new method on our RubySFTP object called last_entry..

def last_entry
  entries[-1]
end

Simple, but more descriptive. This might not matter so much for -1 as anybody working on this code should know that -1 is the last index, but it is something to keep an eye on.

While we're at it, we'll also extract SERVER_BACKUP+files[-1] and LOCAL_BACKUP+files[-1] into descriptive methods..

def last_entry_remote_path
  SERVER_BACKUP + last_entry
end

def last_entry_local_path
  LOCAL_BACKUP + last_entry
end

Now let's tackle the sftp.download! block.

I will not rewrite the whole thing, but you should consider using a custom handle as detailed in the docs here.

That means you would have a custom handler such as:

class CustomHandler
  def on_open(downloader, file)
    puts "starting download: #{file.remote} -> #{file.local} (#{file.size} bytes)"
  end

  def on_get(downloader, file, offset, data)
    puts "writing #{data.length} bytes to #{file.local} starting at #{offset}"
  end
end

..and your sftp call would look like:

sftp.download!(last_entry_remote_path, last_entry_local_path, progress: CustomHandler.new)

That about ends the core of our refactoring. Let's address the log for a second. Effectively what we want is Logger.log('etc'). We can then change the implementation without affecting the code which uses the logger. Here's a simple logger:

class Log
  def self.log(string)
    puts string
  end
end

You can see now how you can change puts to write to a file instead without actual editing the code which uses the logger.


Now let's fix the constants.

I said before that those constants weren't actually constants. Truth is, they ARE constants in the context of your script because they will not change throughout the execution of it. However, if you extract your script into objects which will be defined in different files, you introduce an element of reusability. You can instantiate your RubySFTP class many times, and pass through different variables as you see fit.

For that reason, we should convert those constants to variables and use as such so we can do:

RubySFTP.start(host, user, server_key, remote_path, local_path)

Though this isn't technically a requirement of your program, using objects has still allowed us to clean things up, and will serve you as it grows.

Before we update our class, we should think about reducing the amount of arguments being passed into it in our newly defined interface above. Generally, 3 arguments is enough. 5 is surely too much. One way to combat this might be to pass in an options hash. Having said that, for simplicity, I am going to keep the 5 arguments.

The long initialize method is a code smell. It doesn't look right for a reason - we are passing in too many arguments.

class RubySFTP
  def initialize(host, user, server_key, remote_path, local_path)
    @host = host
    @user = user
    @server_key = server_key
    @remote_path = remote_path
    @local_path = local_path
  end

  def self.start(host, user, server_key, remote_path, local_path)
    new(host, user, server_key, remote_path, local_path).start
  end

  def start
    # etc
  end
end

With all of those changes, here is the finished code, broken down into a few objects - RubySFTP, Entry, and CustomHandler. Keep in mind that this can be broken down further.

class RubySFTP
  def initialize(host, user, server_key, remote_path, local_path)
    @host = host
    @user = user
    @server_key = server_key
    @remote_path = remote_path
    @local_path = local_path
  end

  def self.start(host, user, server_key, remote_path, local_path)
    new(host, user, server_key, remote_path, local_path).start
  end

  def start
    # prime for extraction!
    Logger.log("Connection to Server successful")
    Logger.log("Total #{entries.size.to_i} backupds found on server.")
    Logger.log("etc")
    Logger.log("etc")

    sftp.download!(
      last_entry_remote_path,
      last_entry_local_path,
      progress: CustomHandler.new
    )
  end

  private

  def sftp
    @sftp ||= Net::SFTP.start(@host, @user, keys: [server_key])
  end

  def sorted_entries
    @sorted_entries ||= sftp.dir.entries([@remote_path]).sort_by(&:name)
  end

  def entries
    sorted_entries.map { |entry| Entry.new(entry) }
  end

  def last_entry
    entries[-1]
  end

  def last_entry_remote_path
    @remote_path + last_entry.name
  end

  def last_entry_local_path
    @local_path + last_entry.name
  end
end

Entry

class Entry
  def initialize(entry)
    @entry = entry
  end

  def name
    @entry.name
  end

  def download_size
    @entry.longname.split(' ')[4].to_i
  end
end

CustomHandler -- from here

class CustomHandler
  def on_open(downloader, file)
    puts "starting download: #{file.remote} -> #{file.local} (#{file.size} bytes)"
  end

  def on_get(downloader, file, offset, data)
    puts "writing #{data.length} bytes to #{file.local} starting at #{offset}"
  end

  def on_close(downloader, file)
    puts "finished with #{file.remote}"
  end

  def on_mkdir(downloader, path)
    puts "creating directory #{path}"
  end

  def on_finish(downloader)
    puts "all done!"
  end
end

Couple of final notes - I might have missed a couple of things during this review and refactoring, but I hope it gives you a good foundation to work from. Secondly, I have left some things for you to clean up yourself so you can apply the advice given -- for e.g. files.size.to_i - 2 should be extracted into a total_files method.

Let me know if you have any questions or have noticed something that doesn't add up and I'll try to help.

\$\endgroup\$
  • \$\begingroup\$ Thanks @Damien Roche for detailed explanation. Will go through it. \$\endgroup\$ – Ashish Wadekar Jan 10 '17 at 8:05
  • \$\begingroup\$ No problem @AshishWadekar! Hope you make use of it! \$\endgroup\$ – Damien Roche Jan 10 '17 at 8:58

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.