Unlocking spreadsheet files quickly

Question

I've created a program for my job that unlocks spreadsheet files quickly and efficiently. What we use to have to do before this program is we'd have to go into the system, search for the file, kill the users process containing the file, unlock the file, then the user can finally get back into their spreadsheet. With my new program it first cd's into the directory containing the files, you enter the file number "folio", it searches through a list of locked files using shell commands, then unlocks the specific one for the user.

I'm looking for some critique on my work, I would really like to know what I can do better what I did wrong, etc..

Source:

#!/usr/local/bin/ruby  

require 'fileutils'

module Kernel
    def ls_grep
        `cd '/my/dir'`
        `ls -la|grep -i .lock`
        puts "Enter Folio #"
        @input_folio = gets.chomp
        lockfile = "/my/dir.~lock.#{@input_folio}.ods#"
        if @input_folio =~ /^\d{7}/ 
            if File.exist?( lockfile )
                puts "Unlock file?"
                input = gets.chomp.upcase
                if input == 'Y'
                     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 Foilio number...
                >
                EDE
                ls_grep
            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
            ls_grep
        end
    end
end
ls_grep

Example usage:

Enter Folio #
1234567

 Lockfile not found for Folio # 1234567"

 If the file name doesn't match the folio number, get the file name
 and use that instead of the Foilio number...

Enter Folio #
erwe

 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...

Enter Folio #

Answer 1

• Don't put your method inside Kernel.
• Rename ls_grep to something more descriptive, e.g. unlock_spreadsheet.
• The two shell commands (cd and ls|grep) aren't needed. The code doesn't depend on the current directory, and you're not using the commands' output.
• Don't use an instance variable (@input_folio) as a temporary variable inside a method. Make it a local variable.
• The regex matches anything that begins with sevent digits. Use /\A\d{7}\z/ instead in order to match against the whole string.
• Don't check if the file exists before deleting it, someone might delete it after you check and before you try to delete it. Instead, try to delete it and catch the possible exception.
• Don't use recursion to repeat the method. Use a loop.

                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 Foilio number...
                >
                EDE

Remove the call to gsub:

                puts <<-EDE

 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 Foilio number...

                EDE

You may want to put those strings in global constants to avoid this decrease in indentation in the code.