patternrubyMinor
Unlocking spreadsheet files quickly
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
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:
``
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
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_grepto something more descriptive, e.g.unlock_spreadsheet.
- The two shell commands (
cdandls|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...
>
EDERemove 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...
EDEYou 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...
>
EDEputs <<-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...
EDEContext
StackExchange Code Review Q#111840, answer score: 4
Revisions (0)
No revisions yet.