0
\$\begingroup\$

I recently posted a program I made to unlock spreadsheet files( see also ), I've taken all of your advice and made it more Ruby like, it has no shell commands in it, and uses FileUtils

I personally think it looks a whole lot cleaner and a whole lot more Ruby like. I would like a critique on my work so far, and would like some pointers if possible.

Source:

#!/usr/local/bin/ruby

require 'fileutils'

def spreadsheet_unlocker
    puts "Enter Folio # if unlocking multiple seperate with comma followed by space."
    folios = gets.chomp
    folios.split(', ').each do |input_folio|
        lockfile = "/my/dir.~lock.#{input_folio}.ods#"
        if input_folio =~ /^\d{7}/ 
            if File.exist?( lockfile )
                puts "Unlock file?"
                input = gets.chomp
                    if input =~ /y/i
                     FileUtils.rm( lockfile )
                     puts "File unlocked."
                else
                    puts "You went through that trouble for no reason..."
                end
            else
                puts <<-EDE.gsub(/^\s*>/, ' ') 
                >
                >Lockfile not found for Folio # #{input_folio}"
                >
                >If the file name doesn't match the folio number, get the file name
                >and use that instead of the Folio number...
                >
                EDE
                spreadsheet_unlocker
            end
        else
            puts <<-EDF.gsub(/^\s*>/, ' ')
            >
            >What part of 'Folio #' is hard to understand? 7 DIGITS"
            >
            >If the file name doesn't match the folio number, get the file name
            >and use that instead of the Folio number...
            >
            EDF
            spreadsheet_unlocker
        end
    end
end
spreadsheet_unlocker

Example usage:

Enter Folio # if unlocking multiple seperate with comma followed by space.
1111111, 2222222, 3333333
Unlock file?
yes
File unlocked.
Unlock file?
yes
File unlocked.
Unlock file?
yes
File unlocked.

Enter Folio # if unlocking multiple seperate with comma followed by space.
2345678

 Lockfile not found for Folio # 2345678"

 If the file name doesn't match the folio number, get the file name
 and use that instead of the Folio number...
\$\endgroup\$
1
\$\begingroup\$

Good job listening to feedback. Keep at it.

  • Use two spaces to indent.
  • Name your methods verbs, not nouns (e.g., unlock_spreadsheet).
  • Split on comma and then use strip to remove leading and trailing whitespace.
  • Rather than calling the method recursively, you may want to put a loop do around the method and then break when you're done.
  • If you run Rubocop, you'll see that your method is too long. Instead, you can move each set of actions inside of if and else blocks into their own methods.
\$\endgroup\$
7
  • \$\begingroup\$ Moving the actions into methods sounds like an excellent idea, thanks for the advice! \$\endgroup\$ – 13aal Dec 1 '15 at 0:24
  • \$\begingroup\$ I forgot to mention why not use nouns for methods..? \$\endgroup\$ – 13aal Dec 1 '15 at 0:59
  • \$\begingroup\$ Nouns for Classes and verbs for methods, because methods do things, and classes have things done to them. Also see: stackoverflow.com/a/412870/1935918 \$\endgroup\$ – Dan Kohn Dec 1 '15 at 2:41
  • \$\begingroup\$ Awh, that makes sense, never thought of it like that, what if I turned the if else blocks into a case statement, do you think that would look better? \$\endgroup\$ – 13aal Dec 2 '15 at 13:07
  • \$\begingroup\$ Yes, I would definitely try to avoid nested if/elses. A case statement is great if you have more than a couple elsifs. \$\endgroup\$ – Dan Kohn Dec 2 '15 at 17:00

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.