HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Unlocking spreadsheet files quickly

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
quicklyspreadsheetfilesunlocking

Problem

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 /, ' ')
>
>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 /, ' ')
>
>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

Solution


  • 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 /, ' ') 
                >
                >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.

Code Snippets

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

Context

StackExchange Code Review Q#111840, answer score: 4

Revisions (0)

No revisions yet.