2
\$\begingroup\$

I have a method that accepts user input. Additionally, it masks passwords then the input is a password. It also bypasses the prompt of if an env variable supplies the needed input.

Apparently, my code needs refactoring, but how?

def self.gets(args)
  array = []
  args.each do |arg|
    print "#{arg.to_s.capitalize}: "
    if ENV['AWESOMEAPP_'+arg.to_s.upcase]
      array.push ENV['AWESOMEAPP_'+arg.to_s.upcase]
    else
      if arg.to_s=~/password/
        array.push STDIN.noecho(&:gets).chomp
      else
        array.push STDIN.gets.chomp
      end
    end
    print "\n"
  end
  array
end
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

After reading @user1777136 s answer, I came up with this

module Awesome
  # Has methods that are similar to but extended versions                                                        
  # of core ruby methods                                                                                         
  module Extensions
    # Get multipe inputs from user                                                                               
    # Masks passwords if detected                                                                                
    # Bypasses prompt when ENV variables present                                                                 
    def self.mgets(names)
      names.map do |name|
        sgets(name)
      end
    end

    # single gets                                                                                                
    def self.sgets(name)
      print "#{name.to_s.capitalize}: "
      return env_sgets(name) if ENV['AWESOME_' + name.to_s.upcase]
      return secure_sgets if name.match?(/password/)
      STDIN.gets.chomp
    end

    # get from env variable                                                                                      
    def self.env_sgets(name)
      ENV['AWESOME_' + name.to_s.upcase]
    end

    # secure gets from stdin                                                                                     
    def self.secure_sgets
      STDIN.noecho(&:gets).chomp
    end
  end
end
\$\endgroup\$
1
  • \$\begingroup\$ Nice one! Thats pretty clear now. Academically speaking, you could put the stuff into a class instead of a module and instantiate it with STDIN and ENV in order to abstract yourself from those two globals. \$\endgroup\$ Nov 1, 2017 at 13:15
0
\$\begingroup\$

Granted I know nothing about the inputs and so on. But I see at least two responsibilities of the method:

  1. Printing something
  2. Returning an array of something

You might want to split the loop accordingly, which could give you:

def self.gets(args)
  array = []
  args.each do |arg|
    print "#{arg.to_s.capitalize}: "
    print "\n"
  end
  args.each do |arg|
    if ENV['AWESOMEAPP_'+arg.to_s.upcase]
      array.push ENV['AWESOMEAPP_'+arg.to_s.upcase]
    else
      if arg.to_s=~/password/
        array.push STDIN.noecho(&:gets).chomp
      else
        array.push STDIN.gets.chomp
      end
    end
  end
  array
end

This can help us further down the line. Nested if, if, else can be refactored to elsif. Rubocop can also tell you this. So the following will remove one level of nesting:

def self.gets(args)
  array = []
  args.each do |arg|
    print "#{arg.to_s.capitalize}: "
    print "\n"
  end
  args.each do |arg|
    if ENV['AWESOMEAPP_'+arg.to_s.upcase]
      array.push ENV['AWESOMEAPP_'+arg.to_s.upcase]
    elsif arg.to_s=~/password/
      array.push STDIN.noecho(&:gets).chomp
    else
      array.push STDIN.gets.chomp
    end
  end
  array
end

Furthermore it looks like we are always returning something from the second loop. So building a new "collector" array (which has a not so great name of array) can be avoided. Which gives us:

def self.gets(args)
  args.each do |arg|
    print "#{arg.to_s.capitalize}: "
    print "\n"
  end
  args.map do |arg|
    if ENV['AWESOMEAPP_'+arg.to_s.upcase]
      return ENV['AWESOMEAPP_'+arg.to_s.upcase]
    elsif arg.to_s=~/password/
      return STDIN.noecho(&:gets).chomp
    else
      return STDIN.gets.chomp
    end
  end
end

Since Ruby will return the last evaluated thing in the method and we are mentioning args twice now we can even chain this:

def self.gets(args)
  args
    .each { |arg| print "#{arg.to_s.capitalize}: \n" }
    .map do |arg|
      if ENV['AWESOMEAPP_'+arg.to_s.upcase]
        return ENV['AWESOMEAPP_'+arg.to_s.upcase]
      elsif arg.to_s=~/password/
        return STDIN.noecho(&:gets).chomp
      else
        return STDIN.gets.chomp
      end
    end
end

Finishing this off by extracting a method or two will lead us to:

def self.gets(args)
  args
    .each(&:print_arg)
    .map(&:read_arg)
end

def print_arg(arg)
  print "#{arg.to_s.capitalize}: \n"
end

def read_arg(arg)
  if ENV["AWESOMEAPP_#{arg.to_s.upcase}"]
    ENV["AWESOMEAPP_#{arg.to_s.upcase}"]
  elsif arg.to_s =~ /password/
    STDIN.noecho(&:gets).chomp
  else
    STDIN.gets.chomp
  end
end

Now I am pretty sure I have broken your code, but this can give you an insight into what you could do.

\$\endgroup\$
2
  • \$\begingroup\$ you certainly broke the behavior, but your use if maps gives me some inspiration \$\endgroup\$ Oct 31, 2017 at 17:17
  • 1
    \$\begingroup\$ the print "#{arg.to_s.capitalize}: " has to happen just before gets, because it's a prompt. IF we have ten prompts, we shouldn't batch print, as I said the prompt should print and then gets \$\endgroup\$ Oct 31, 2017 at 17:18

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.