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.
