# Cancel those hung print jobs

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]

def scan_for_jobs
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)
column = 0
job_ids = res.lines.map { |job| job.split(/\s+/)[0] }.each do |task_id|
end
end

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
Cancelling job: 254128
Cancelling job: 254129
Cancelling job: 254144
Cancelling job: 254145
Cancelling job: 254143

No print jobs on server

• This is pretty cool. You should be able to use the variable that you set the Net::SSH too instead of calling it twice, try passing it as an argument to the method, call it, and do an exec! on it. Jan 7 '16 at 1:11
• This isn't really OOP. Jan 8 '16 at 22:50
• @EBrown A for effort lol Jan 9 '16 at 19:16

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

end

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

• I can tell you why they use nil, net/ssh has a required password argument, don't ask me why, I don't know, but passing it nil gives it the required arguments along with the no sensitive information.. Jan 9 '16 at 5:43
• @Ekult3k I was actually referring to the line if info == nil. Jan 9 '16 at 6:25
• oh, I'm guessing that if the prt_jobs command shows nothing then it says no print jobs on server, that's kind of obvious isn't it..? What happens in bash is when you call the command prt_jobs if there's none on the server it doesn't return anything.. So if info == nil output that there's no hung print jobs on the server... Jan 9 '16 at 19:15
• @Ekult3k No, I get what OP is trying to do, but if prt_jobs produces no output then exec! returns the empty string, which is distinct from nil, so the if condition is never satisfied. I'll try to edit my answer to be more clear. Jan 9 '16 at 22:19
• @Bam That's interesting - I can't get exec! to ever return anything but a String but if you're getting the No print jobs on server message when appropriate (that's the only part I was calling into question, not whether you could cancel jobs) then I must be mistaken. Sorry to cause confusion. Jan 11 '16 at 22:23

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

• Why do you have to use the !? I thought it would work either way
– Bam
Jan 6 '16 at 19:13
• @Bam .strip does not modify the value stored in the variable, only methods ending with ! modify the variable (and very few others) Jan 6 '16 at 19:17
• @Caridorc I don't think this is a bug, more of a useless call, it doesn't matter if there's a space because bash will ignore trailing white space, won't it? Jan 7 '16 at 1:13