Cancel those hung print jobs

Question

I've created a program that will list all hung print jobs on a specified server, from there it will load them and strip the jobs down to their Job ID then cancel the jobs on that server only.

I would like a critique of my work please.

• How does this look as an OOP script?
• Is there anything I can do differently?
• Is calling the Net::SSH.start twice really necessary?

Source:

#!/usr/local/bin/ruby

require 'rubygems'
require 'net/ssh'
require 'etc'

class PrintJobs

  HOST = ARGV[0]
  USERNAME = Etc.getlogin
  PASSWORD = nil

  def scan_for_jobs
    check_jobs = Net::SSH.start(HOST, USERNAME, :password => PASSWORD) do |ssh|
      cmd = "prt_jobs"
      info = ssh.exec!(cmd)
      if info == nil
        puts "No print jobs on server #{HOST}"
      else
        res = info.split("\n").reject {|line| line.match(/\s+2016\s+/)}.join("\n")
        puts res
        print "Process into kill que: "
        input = STDIN.gets.chomp.upcase
        if input == "YES"
          kill_que(check_jobs, res)
        else
          exit 1
        end
      end
    end
  end

  def kill_que(check_jobs, res)
    puts "Loading jobs into kill que.."
    column = 0
    job_ids = res.lines.map { |job| job.split(/\s+/)[0] }.each do |task_id|
      kill_jobs(task_id)
    end
  end

  def kill_jobs(task_id)
    execute = Net::SSH.start(HOST, USERNAME, :password => PASSWORD)
    id_to_strip = task_id.gsub(/\-/," ")
    column = 1
    stripping_id = id_to_strip.lines.map { |task| task.split(/\s+/)[1] }.each do |id|
      id.strip
      puts "Cancelling job: #{id}"
      #`sudo cancel #{id}`
      execute.exec!("sudo cancel #{id}")
    end
  end
end

killer = PrintJobs.new
killer.scan_for_jobs

Usage example:

closing_2-254128        root            933888   Thu 03 Dec 2015 03:49:13 PM CST
closing_2-254129        root            933888   Thu 03 Dec 2015 03:50:16 PM CST
laser42-254144          root            192512   Thu 03 Dec 2015 04:24:02 PM CST
laser42-254145          root            171008   Thu 03 Dec 2015 04:25:10 PM CST
ship_audit-254143       root            141312   Thu 03 Dec 2015 04:23:13 PM CST
Process into kill que: yes
Loading jobs into kill que..
Cancelling job: 254128
Cancelling job: 254129
Cancelling job: 254144
Cancelling job: 254145
Cancelling job: 254143

No print jobs on server 

Answer 1

Wrapping your methods in a class definition isn't the same thing as making your project OOP. These methods don't care about their receiver, and forcing the programmer to instantiate the PrintJobs class before calling them doesn't accomplish anything useful here. Now, for special-purpose scripts like this, not being OO isn't the worst thing in the world - in fact, going OO can be design overkill for a project this small. However, if you go the functional route, tell it like it is - refactor class PrintJobs to module PrintJobs (because Module is, idiomatically, the correct type for a collection of associated methods) so that you can do your invocation with PrintJobs.scan_for_jobs.

If you want to convert the code to be more OO - to make it easier to incorporate into another project, for example - objects need to be responsible for the data they operate on. That means that the HOST/USERNAME/PASSWORD values need to be instance variables, not class constants. You also want to be sure that your data is typed appropriately. Currently, there's a lot of passing around and manipulation of strings that represent print jobs. If the output format of prt_jobs ever changes, we'll need to update every piece of code that interacts with these objects. Abstracting them into their own type will help prevent this.

Stylistic touches:

• Get rid of extraneous definitions - cmd is defined and then only used once, in the immediately following line. Some other variables are defined and then never used.
• Avoid abbreviation. I assume res means "results", but I'm not sure.
• Exiting with a non-zero status code should only happen if the program encounters some sort of error, which isn't the case with your usage. I'd remove the statement entirely, since there's no real reason for PrintJobs to be responsible for program termination anyway.
• Last but not least, the word is "queue".

Here's the class refactored, with additional comments in-line. Since I don't have a print server handy the code has not been tested and probably contains bugs, but it should give you an idea of what a more object-oriented structure looks like.

class PrintManager
  # I've made the PrintJob class belong to PrintManager for now,
  # but you could move it to global scope if it becomes handy elsewhere
  class PrintJob
    attr_reader :printer, :id, :user, :some_other_thing, :date

    def initialize(printer, id, user, some_other_thing, date)
      @printer, @id, @user = printer, id, user
      @some_other_thing, @date = some_other_thing, date
    end

    # If the output of `prt_jobs` ever changes, this is the only code you'll
    # need to update.
    # In real life we'd want to parse the time into a proper Time object.
    def self.parse(job_string)
      job_string =~ /^(\S+)-(\d+)\s+(\S+)\s+(\d+)\s+(.+?)\s*$/
      return self.new($1, $2.to_i, $3, $4.to_i, $5)
    end

    def to_s
      "#{@printer}-#{@id}\t#{@user}\t#{@some_other_thing}\t#{@date}"
    end
  end

  def initialize(host, username, password)
    @host, @username, @password = host, username, password
  end

  def scan_for_jobs
    Net::SSH.start(@host, @username, :password => @password) do |ssh|
      info = ssh.exec!("prt_jobs")
      if info == nil
        puts "No print jobs on server #{@host}"
      else
        # The old "res" object we got here was a big blobby String.
        # Now we have an Array of PrintJobs, which is much easier
        # to interact with.
        print_jobs = info.split("\n").map {|line|
          PrintJob.parse(line)
        }
        print_jobs.reject! {|job| job.date.match(/\s+2016\s+/) }

        # One disadvantage of parsing out the string at object
        # creation is that we lose formatting, and our tab stops
        # may not line up without some extra effort. Oh well!
        puts print_jobs
        print "Kill these jobs (yes/no): "
        input = STDIN.gets.chomp.upcase
        if input == "YES"
          # The kill_queue method only really existed to do string
          # parsing. We accidentally made it unnecessary when we made
          # PrintJob a real class.
          print_jobs.each { |job| kill_job(job, ssh)}
        end
      end
    end
  end

  # Since kill_job is only called from within scan_for_jobs, we'll
  # just reuse its SSH object. Depending on how your program grows, it
  # may be better to have kill_job open its own SSH connection like
  # you had originally, or make it an attribute of PrintManager, or
  # something else. 
  def kill_job(job, ssh)
    # Got to remove a bunch more string parsing here
    puts "Cancelling job: #{job.id}"
    ssh.exec!("sudo cancel #{job.id}")
  end
end

killer = PrintManager.new(ARGV[0], Etc.getlogin, nil)
killer.scan_for_jobs

Answer 2

Bug / useless line

In the block of code:

  id.strip
  puts "Cancelling job: #{id}"
  #`sudo cancel #{id}`
  execute.exec!("sudo cancel #{id}")

id.strip generates a value to be immediately discarded, essentially a no-op.

In Ruby immutability is the default: look at the following IRB session for clarification:

irb(main):001:0> x= " foo"
=> " foo"
irb(main):002:0> x.strip
=> "foo"
irb(main):003:0> x
=> " foo"
irb(main):004:0> x.strip!
=> "foo"
irb(main):005:0> x
=> "foo"

The line does not actually strip the name causing a bug -> use the in-place version id.strip! (Note the final !)