SFTP Ruby script for downloading files from server

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

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:

```

StackExchange Code Review Q#151041, answer score: 7