2
\$\begingroup\$

According to the local cop (rubocop), my method has too many lines.

lib/awesomelibrary/tunnelable.rb:4:5: C: Method has too many lines. [17/10]
    def tunnel_run(cmd) ...
    ^^^^^^^^^^^^^^^^^^^

Below is the method. How do I refactor it?

module AwesomeLibrary
  # Gives object the ability to execute bash commands on itself via ssh                                          
  module Tunnelable
    def tunnel_run(cmd)
      retries = 0
      code = nil
      Net::SSH.start(@tunnel_ip, @tunnel_username, keys: @tunnel_key_name, verify_host_key: false) do |ssh|
        the_channel = ssh.open_channel do |channel|
          channel.exec cmd do |ch, success|
            raise "could not execute command" unless success
            ch.on_data { |_c, data| print data }
            ch.on_extended_data { |_c, _type, data| print data }
            ch.on_request("exit-status") { |_ch, data| code = data.read_long }
          end
        end
        the_channel.wait
      end
      abort "#{cmd} returned #{code} !!" if code != 0
    rescue Net::SSH::ConnectionTimeout
      puts "Net::SSH::ConnectionTimeout"
      retry if (retries += 1) < 3
    end
  end
end

I'm using this is the library: Net::SSH 4.x.

\$\endgroup\$
  • 2
    \$\begingroup\$ You don't necessary need to refactor anything. Sure, it could probably be made prettier by one metric or another, but.. eh'. I'd be more concerned that it'll retry forever, since retry will start the method over, setting retries = 0 each time... \$\endgroup\$ – Flambino Oct 31 '17 at 15:05
  • 2
    \$\begingroup\$ As much as I hate to be "that guy", letting a script tell you that your method has too many lines is a terrible reason to refactor. Does it work? Does it make sense? Is it testable? Those are the important questions, not "how many lines does it have?". \$\endgroup\$ – Der Kommissar Oct 31 '17 at 15:19
1
\$\begingroup\$

In my opinion, your method does not have too many lines, but the line calling Net::SSH.start is too long.

My terminals and text editors are 80 characters wide, so I would split the long line into 2 lines

      Net::SSH.start(@tunnel_ip, @tunnel_username, keys: @tunnel_key_name,
                     verify_host_key: false) do |ssh|

If you must reduce the number of lines, you can move some lines into new methods. The extra method calls will make spaghetti of the control flow, so the code will be difficult to read and understand, but it might satisfy your cop.

    def tunnel_run(cmd)
        retries = 0
        code = tunnel_run_command(cmd)
        abort ... if code != 0
    rescue Net::SSH::ConnectionTimeout
        ...
    end

    private
    def tunnel_run_command(cmd)
        code = nil
        Net::SSH.start(...) do |ssh|
            the_channel = ssh.open_channel do |channel|
                channel.exec do |ch, success|
                    code = tunnel_run_channel(ch, success)
                end
            end
            the_channel.wait
        end
        code
    end

    def tunnel_run_channel(ch, success)
        code = nil
        raise ... unless success
        ch.on_data { ... }
        ch.on_extended_data { ... }
        ch.on_request("exit-status") { ... }
        code
    end

Beware that the private methods tunnel_run_command and tunnel_run_channel will pollute the method namespace in all objects that extend Tunnelable. Privacy in Ruby is by object, so the object inherits the private methods from Tunnelable, and Ruby allows the object to call those methods.

The names tunnel_run_command and tunnel_run_channel must not conflict with other methods of the object.

\$\endgroup\$

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.