Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have a method to upload a file using the Net::SFTP gem:

class Uploader

 def initialize(host,user,password)
   @host = host
   @user = user
   @password = password
 end

 def upload(local_file_path,remote_file_path)
  Net::SFTP.start(@host,@user,:password => @password) do |sftp|
   sftp.upload!(local_file_path,remote_file_path) do |event,uploader,*args|
     case event
     when :open
       Rails.logger("Starting upload...")
     when :finished
       Rails.logger("Finishing upload...")
     end
   end
  end
 end
end

The upload method seems rather large to me. Any suggestions on how I could split it up into smaller components?

share|improve this question

I don't see much reason to worry about this method's length. At least not right now - if you add more event handling, yeah, it might become a little cumbersome. But for what it does now, there's no pressing need to do anything.

I would recommend more whitespace, though. There's no reason to compress,every,line,that,has,commas,in,it. Just makes it harder to read.

I'm also fairly sure that the :finished event is actually called :finish

Structurally, just for the sake of it, you could do something like:

class Uploader
  attr_reader :host, :user, :password

  def initialize(host, user, password)
    @host = host
    @user = user
    @password = password
  end

  def upload(local_file_path, remote_file_path)
    Net::SFTP.start(host, user, password: password) do |sftp|
      sftp.upload!(local_file_path, remote_file_path) do |event, uploader, *args|
        send(event, uploader, *args) if respond_to?(event, true)
      end
    end
  end

  private

  def open(uploader, *args)
    Rails.logger("Starting upload...")
  end

  def close(uploader, *args)
    Rails.logger("Upload complete")
  end

  def finish(uploader, *args)
    Rails.logger("All done")
  end
end

Basically, we're handling events with methods instead of a case block. I've added the close method/event handler as an example.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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