8
\$\begingroup\$

I've created a program that will pull spreadsheets out of a large tarball.

First it specifies the file name through the "folio" number which is given as an argument when the program is called. From there it runs a bash command that will list a text file containing the information needed to find the tarball, the information needed is a date. It then collects the information finds the tarball, runs another bash command to unarchive the spreadsheet from the tarball, and then moves the spreadsheets.

I would like some critique on my work. Are there better ways to extract from a tar? Would this be better as a module then a class?

I know it doesn't delete the directory that is created from extracting the spreadsheets, mostly because I don't trust myself enough to remove a directory called home.

#!/usr/local/bin/ruby

require 'rubygems'
require 'colored'
require 'fileutils'

class ArchiveManager

  attr_accessor :folio_num

  def initialize(folio_num)
    @folio_num = folio_num
    @src_dir = "/home/<dir>/<user>/<dir>/home/common/4000_UW_spreadsheets/#{folio_num}.ods"
    @dst_dir= "/home/<dir>/<user>/<dir>"
  end

  def num_check
    if folio_num =~ /^\d{4,7}$/
      search_for_spreadsheet
    else
      puts
      puts "                  Invalid Folio length".red.bold,
       "        USAGE: unarchive_ss <folio number 4-7 digits>".red.bold
      puts
    end
  end

  def search_for_spreadsheet
    location = FileUtils.chdir("/home/<dir>/<user>/<dir>")
    digits = `grep -lr #{@folio_num}.ods *.txt`
    file_name = digits.match(/\d+/).to_s
    puts "Spreadsheet found in UW_archive_#{file_name}.tgz".green.bold
    extract(file_name, location)
  end

  def extract(file_name, location)
    if File.exist?("#{@dst_dir}/#{@folio_num}.ods")
      found_error = raise SecurityError.new("File: #{@folio_num} exists within directory: #{@dst_dir}; will not continue..")
      abort("File exists in directory, either use file as parachute, or get rid of it..".red.bold)
    else
      tar_location = "home/common/4000_UW_spreadsheets"
      `tar -x -f UW_archive_#{file_name}.tgz #{tar_location}/#{@folio_num}.ods`
      move(tar_location, location)
    end
  end

  def move(tar_location, location)
    if File.exist?("#{@src_dir}")
      puts "File extracted, and being moved.".green.bold
      sleep(1)
      FileUtils.mv("#{@src_dir}", "#{@dst_dir}")
      puts "File: #{@folio_num}.ods, is now located in directory path: #{@dst_dir}".blue.bold
      puts "Remove created directories manually".red.bold
    else
      puts "Unable to locate file: #{@folio_num}.ods; source not found.".red.bold
      src_error = raise LoadError.new("Unable to find file: #{@folio_num}.ods; in directory #{@src_dir}")
    end
  end
end


ss = ArchiveManager.new(ARGV[0])
ss.num_check

Example of usage: File 2715015

Running..

Finished running..

\$\endgroup\$
  • \$\begingroup\$ A few questions: 1. Are you going to distribute this code so other people can use it, or is it for your personal use? (bin stub in gem vs something.rb) 2. Is the archive format fixed, or do you want to present a menu of items in the tarball for users to select from? (shell vs zlib) 3. Are you going to need to extend this to handle different configurations for the destination, validation of file name, source locations? \$\endgroup\$ – randym Feb 4 '16 at 15:26
  • \$\begingroup\$ @randym 1. Haven't decided yet 2. Yes it is a fixed format, it will only display the tarball that the file is inside of. 3. That's a good question, I'll get back to you on that one, because I'm not sure. \$\endgroup\$ – Bam Feb 4 '16 at 19:22
  • \$\begingroup\$ @randym Alright an answer to your third question, I will extend it to handle different configs, I'm just looking for some critique on what's happening with it right at this moment \$\endgroup\$ – Bam Feb 5 '16 at 16:41
1
\$\begingroup\$

Your main questions

Are there better ways to extract from a tar?

Using shell commands, no. Well, with any modern version of tar, instead of tar -x -f archive file you could write simpler as tar xf archive file.

I don't know Ruby, but probably there exist libraries to extract files from archives. It would be better to use that.

The current script is a mixture of Ruby and Bash commands. This is a fragile setup:

  • It requires two technologies to run
  • It requires understanding two technologies

The solution would be more robust if it was written either completely in Ruby or in Bash.

I know it doesn't delete the directory that is created from extracting the spreadsheets, mostly because I don't trust myself enough to remove a directory called home.

You could recursively remove the empty parent directories. That should be safe.

Naming

Looking at this usage:

ss = ArchiveManager.new(ARGV[0])
ss.num_check

This doesn't really tell what's going to happen. The program extracts a file from an archive. This would look like a more natural usage:

archive_manager = ArchiveManager.new()
archive_manager.extract_file(ARGV[0])

Design

The naming issue brought to the fore some design issues. The naming change I suggested earlier for more natural usage, also forces further changes that will lead to better design:

  • Currently the ArchiveManager class is one-time use to extract a single file. That's not a natural way to use classes. Making the ArchiveManager reusable by adding a extract_file method with a file parameter is more natural.

  • Currently many methods of ArchiveManager depend on the folio_num stored in the class. Methods are easier to understand when their dependencies are more directly visible, for example as input parameters. It also makes them easier to test.

| improve this answer | |
\$\endgroup\$

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.