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'
#Date : 18 December 2016

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

HOST = "www.example.com"

#User for connection

#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

#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

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

case event
when :open then
puts "Perhaps you can sip a coffee till then!"
when :get then
# args[1] : byte offset in remote file
# args[2] : data that was received

when :close then
puts "Done! Closing connection to server."
when :mkdir then
# args[0] : local path name
puts "Creating directory #{args[0]}"
when :finish then
end
end
end


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
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

# 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

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


Now our start method simply has:

def start
# 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
end

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")

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

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


CustomHandler -- from here

class CustomHandler
end

puts "writing #{data.length} bytes to #{file.local} starting at #{offset}"
end

puts "finished with #{file.remote}"
end

puts "creating directory #{path}"
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.