# Unlocking spreadsheet files quickly

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 #


## 1 Answer

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

• Local variables are \$variable correct? What do you suggest instead of kernel should I make it into a class instead? Also what do you mean by check for the possible exception? – 13aal Nov 28 '15 at 1:01
• Just variable, like lockfile. Make ls_grep a global method: simply def ls_grep; ...; end not inside anything. Instead of the File.exist? condition, wrap the deletion inside a rescue block: begin; FileUtils.rm(lockfile); puts "success"; rescue Errno::ENOENT; puts "No such file"; end (semicolons are newlines) – Spike Nov 28 '15 at 16:25
• The indents in the gsub are on purpose – 13aal Nov 28 '15 at 23:39