Ruby programming styles and new program?

ruby-talk
1

Hello,
   As a newbie to Ruby, I've just written a program that uses Ruby for
caller ID services on a WinXP machine. It seems to work okay, but I come
from C/++, and wonder if I really did the program the way Ruby is
generally done. Any suggestions on making this more Ruby-like? In
particular, I'm still unsure about variable scoping rules. Does anyone
have a link to a document that explains Ruby variable scoping well?
Also, in Java or C#, the program logic is generally wrapped in a
class-is this also the way things are done in ruby, or is procedural
logic okay?

=========== cut ===========
#!/usr/bin/ruby -w

···

#########################################################
#
# log_CID.rb
#
# Ruby CID logging script
#
# Logs all activity at a modem set to decipher CID
# (Caller ID) information. Backs up weekly also.
#
##########################################################

require 'date'
require 'zip/zip'
require 'serialport/serialport.so'

############################################
#
# local settings here.
#
$backup_zip_filename = "CID_Data.zip"
modem_init_string = "AT+VCID=1\r\n" # for USR verbose CID output
backup_dir = "c:/modemlog"
base_log_name = "CID"
wday_to_backup = 7
hr_to_backup = 2
port_name = 'COM3:'
days_before_archive = 7
# const for backup intervals
ARCHIVE_DAYS_SECS = 60 * 60 * 24 * days_before_archive
# debug on or off?
DEBUG = true
#
##############################################

# var for calendar based backup, start with invalid value.
last_backup_day = 367

#################################
# subroutines

def get_current_fname(backup_dir, base_log_name)
     t = Time.now
     fname = backup_dir + '/' + base_log_name +
       t.strftime("%Y%m%d") + ".log"
     return fname
end

def archive_old_to_zip(backup_dir, base_log_name, t)
     moved = 0
     Dir.foreach(backup_dir) {
         > logfile |
         if(logfile =~ /^CID/)
             ftime = File.stat(logfile).mtime
             if(t > ftime + ARCHIVE_DAYS_SECS)
                 moved += 1 if move_to_archive(logfile)
             end
         end
     }
     return moved
end

def move_to_archive(fname)
     Zip::ZipFile.open($backup_zip_filename, 1) {
         > zfile |
         zfile.add(fname, fname)
     }
     File.delete(fname)
end

def log_text(bckup_dir, bse_log_name, txt)
     log_name = get_current_fname(bckup_dir, bse_log_name)
     logfile = File.new(log_name, "a")
     logfile.print(txt)
     logfile.close
end

###############################
# begin main program

# move to the dir for backup
Dir.chdir(backup_dir)

# Open the port and set up CID via modem_init_string.
port = SerialPort.new(port_name)
port.read_timeout = 0
port.puts(modem_init_string)

print "Starting run with port ", port_name,
   " and logging to dir ", backup_dir, "\n" if DEBUG

# Loop with pauses to look for data at the port we can record.
while(true)
     while(text = port.gets)
         print text if DEBUG
         # squeeze double \r, etc to just \n
         text.sub!(/[\r\n]+/, "\n")
         # log text unless it is just spaces
         if(text =~ /\S/)
             log_text(backup_dir, base_log_name, text)
             # archive old logs daily
             t = Time.now
             yday = t.yday
             if(yday != last_backup_day)
                 archive_old_to_zip(backup_dir, base_log_name, t)
                 last_backup_day = yday
             end
         end
     end
     if(DEBUG)
         msg = Time.now.to_s +
           ": dropped out of system call, restarting loop.\n"
         print msg
         log_text(backup_dir, base_log_name, msg)
     end
end

return 0

========================================

2

Hello,
   As a newbie to Ruby, I've just written a program that uses Ruby for
caller ID services on a WinXP machine. It seems to work okay, but I come
from C/++, and wonder if I really did the program the way Ruby is
generally done. Any suggestions on making this more Ruby-like? In
particular, I'm still unsure about variable scoping rules. Does anyone
have a link to a document that explains Ruby variable scoping well?
Also, in Java or C#, the program logic is generally wrapped in a
class-is this also the way things are done in ruby, or is procedural
logic okay?

=========== cut ===========
#!/usr/bin/ruby -w

#########################################################
#
# log_CID.rb
#
# Ruby CID logging script
#
# Logs all activity at a modem set to decipher CID
# (Caller ID) information. Backs up weekly also.
#
##########################################################

require 'date'
require 'zip/zip'
require 'serialport/serialport.so'

############################################
#
# local settings here.
#
$backup_zip_filename = "CID_Data.zip"
modem_init_string = "AT+VCID=1\r\n" # for USR verbose CID output
backup_dir = "c:/modemlog"
base_log_name = "CID"
wday_to_backup = 7
hr_to_backup = 2
port_name = 'COM3:'
days_before_archive = 7
# const for backup intervals
ARCHIVE_DAYS_SECS = 60 * 60 * 24 * days_before_archive
# debug on or off?
DEBUG = true
#
#
##############################################

# var for calendar based backup, start with invalid value.
last_backup_day = 367

#################################
# subroutines

class BackupClass
  attr_reader :backup_dir, :base_log_name
  def initialize(backup_dir, base_log_name)
     @backup_dir = backup_dir
     @base_log_name = base_log_name
  end

def get_current_fname(backup_dir, base_log_name)
     t = Time.now
     fname = backup_dir + '/' + base_log_name +
       t.strftime("%Y%m%d") + ".log"
     return fname
end

def current_fname
    "#{backup_dir}/#{base_log_name}#{Time.now.strftime("%Y%m%d")}.log"
end

···

On Mon, 11 Oct 2004 09:04:40 +0900, Bill <bi11@lynxview.com> wrote:

def archive_old_to_zip(backup_dir, base_log_name, t)
     moved = 0
     Dir.foreach(backup_dir) {
         > logfile |
         if(logfile =~ /^CID/)
             ftime = File.stat(logfile).mtime
             if(t > ftime + ARCHIVE_DAYS_SECS)
                 moved += 1 if move_to_archive(logfile)
             end
         end
     }
     return moved
end

#
# I'm not entirely sure about the inject rewrite here; just
# accumulating on moved may be just as good.
#
def archive_old_to_zip(backup_dir, base_log_name, t)
     dir = Dir.open(backup_dir)
     moved = dir.inject(0) do |lcount, ogfile|
         # Skip-logic can use trailing conditionals
         next unless logfile =~ /^CID/
         next unless t > File.stat(logfile).mtime + ARCHIVE_DAYS_SECS
         # Active logic use prefix if
         if (move_to_archive(logfile))
             next count + 1
         else
             next count
         end
     end
     dir.close
     return moved
end

# I don't like the $backup_zip_filename below; should come from
# the object.

def move_to_archive(fname)
     Zip::ZipFile.open($backup_zip_filename, 1) {
         > zfile |
         zfile.add(fname, fname)
     }
     File.delete(fname)
end

def log_text(bckup_dir, bse_log_name, txt)
     log_name = get_current_fname(bckup_dir, bse_log_name)
     logfile = File.new(log_name, "a")
     logfile.print(txt)
     logfile.close
end

def log_text(txt)
     logfile = File.new(current_fname, "a")
     logfile.print(txt)
     logfile.close
end

# ... and continue refactoring into an object.

I can do another pass if you finish the refactoring job.

Eivind.

3

Okay--I think this is the last revision unless there's a feature
request. Thanks to all who helped with the Ruby/Smalltalk idioms.

···

==========================
#!/usr/bin/ruby -w
#########################################################
#
# log_CID.rb
#
# Ruby CID logging script
#
# Logs all activity at a modem set to decipher CID
# (Caller ID) information. Archives the daily logs as well.
#
##########################################################

require 'zip/zip'
require 'serialport/serialport.so'
require 'yaml'
require 'getoptlong'

#################################

class ModemCIDMonitor
     def initialize(config, logger)
         @port_name = config.port_name
         @MAX_PORT_ERRORS = config.max_port_errors
         @debug = config.debug
         @log_blank_lines = config.log_blank_lines
         @modem_init_string = config.modem_init_string
         @logger = logger
         @port_err_count = 0
         @port = SerialPort.new(@port_name)
         @port.read_timeout = 0
         @port.puts(@modem_init_string)
     end

     def log(txt)
         print txt if @debug
         @logger.log_text(txt)
     end

     def run
         print "Starting run with port ", @port_name,
           " and logging to dir ", @logger.archive_dir, "\n"
         loop do
             @port.each_line do | text |
                 next unless text =~ /\S/ or @log_blank_lines
                     # squeeze double \r, etc to just \n
                     text.sub!(/[\r\n]+/, "\n")
                     log(text)
             end
             msg =
              "#{Time.now.to_s}: dropped out of system call, restarting
loop.\n"
             log(msg) if @debug
             @port_err_count += 1
             if(@port_err_count > MAX_PORT_ERRORS)
                 errmsg = "Too many port errors...ending run\n"
                 log(errmsg)
                 return errmsg
             end
         end
     end
end

class CID_Config
     attr_reader :config_file, :as_hash, :defaults

     def initialize(config_file)
         @config_file = config_file
         @as_hash = YAML::load( File.open(@config_file) )
         @defaults = {
                             'archive_dir' => './',
                             'base_log_name' => 'CID',
                             'debug' => false,
                             'archive_days_interval' => 1,
                             'archive_zip_filename' => 'CID_archive.zip',
                             'port_name' => 'COM1',
                             'max_port_errors' => 100,
                             'log_blank_lines' => false,
                             'modem_init_string' => "AT+VCID=1\r\n",
                         }
         @defaults.each do | k, v |
             @as_hash[k] = v unless @as_hash.has_key?(k)
         end
     end

     def method_missing(method, *args)
         method_key = method.to_s
         if args.length == 0 and @as_hash.has_key?(method_key)
             return @as_hash[method_key]
         else
             super
         end
     end

end

class DailyLogWithArchive
     attr_reader :archive_dir, :base_log_name, :archive_zip_filename,
       :archive_days_secs, :archive_days_interval
     attr_accessor :debug

     def initialize(config)
         @archive_dir = config.archive_dir
         @base_log_name = config.base_log_name
         @debug = config.debug
         @archive_days_interval = config.archive_days_interval
         @archive_zip_filename = config.archive_zip_filename
         @last_archive_day = -1
         @secs_before_archive = 60 * 60 * 24 * config.days_before_archive
     end

     def current_fname
         "#{archive_dir}/#{base_log_name}#{Time.now.strftime('%Y%m%d')}.log"
     end

     def logfile_needs_moving(logfile)
         # check if the logfile mtime is old enough to archive the file
         return false unless logfile.index(@base_log_name) == 0
         return false unless Time.now >
           File.stat(logfile).mtime + @secs_before_archive
         return true
     end

     def archiving_needed
         # check if we should do periodic archiving (up to daily)
         yday = Time.now.yday
         return false unless yday != @last_archive_day
         return false unless
           yday >= @last_archive_day + @archive_days_interval or
           yday == 0
         return true
     end

     def archive_old_to_zip
         moved = 0
         Dir.chdir(@archive_dir) do
             dir = Dir.open('.')
             moved = dir.inject(0) do | move_count, logfile |
                 next unless logfile_needs_moving(logfile)
                 next unless move_to_archive(logfile)
                 log_text("LOGGER: Archiving file " + logfile + "\n") if
@debug
                 next move_count + 1
             end
         end
         return moved
     end

     def move_to_archive(fname)
         Zip::ZipFile.open(@archive_zip_filename, 1) {
             > zfile |
             return nil if zfile.exist?(fname)
             zfile.add(fname, fname)
         }
         rc = File.delete(fname)
         return true if rc and rc == 1
         return nil
     end

     def log_text(txt)
         Dir.chdir(@archive_dir) do
             File.open(current_fname, "a") do | logfile |
                 logfile.print(txt)
             end
             if(archiving_needed)
                 archive_old_to_zip
                 @last_archive_day = Time.now.yday
             end
         end
     end

end

###############################

config_yaml = 'log_CID.yml' # default config file
opts = GetoptLong.new( [ "--conf", "-c", GetoptLong::REQUIRED_ARGUMENT ] )
opts.each do | opt, arg |
     if(opt == "--conf" and arg.length > 0)
         config_yaml = arg
         last
     end
end

config = CID_Config.new(config_yaml)
logger = DailyLogWithArchive.new(config)
monitor = ModemCIDMonitor.new(config, logger)

# no return from run unless abort on port timeouts or errors
errmsg = monitor.run

print errmsg

return 0

==========================

############################################
#
# log_CID YAML config file.
#

# name of archive file--daily logs are moved to this archive
archive_zip_filename: "CID_Data.zip"

# modem initialization string.
# need to set to log verbose caller ID information (+VCID=1 or #CID=1, etc)
# also need to set to NOT answer, just monitor line (usually the default)
modem_init_string: "AT+VCID=1\r\n"

# directory to kep log files
archive_dir: "c:/modemlog"

# base log name for daily log files
# daily log file name is this, plus YYYYMMDD date, plus .log extension
# eg. CID20041004.log
base_log_name: CID

# the comm port having the CID-capable modem
port_name: 'COM1:'

# days that a daily log file is kept prior to archiving the file
days_before_archive: 7

# maximum port read errors allowed before aborting run
max_port_errors: 3000

# whether to log whitespace-only lines
log_blank_lines: false

# debug on or off?
debug: true

#
# end YAML config file.
#
#####################################################

4

Eivind Eklund wrote:

#
# I'm not entirely sure about the inject rewrite here; just # accumulating on moved may be just as good.
#
def archive_old_to_zip(backup_dir, base_log_name, t)
     dir = Dir.open(backup_dir)
     moved = dir.inject(0) do |lcount, ogfile|
         # Skip-logic can use trailing conditionals
         next unless logfile =~ /^CID/
         next unless t > File.stat(logfile).mtime + ARCHIVE_DAYS_SECS
         # Active logic use prefix if
         if (move_to_archive(logfile))
             next count + 1
         else
             next count
         end
     end
     dir.close
     return moved
end

Thanks for the suggestion to put the logging/archiving code in a class.
That refactoring does allow more appropriate data scoping.

One problem: I cannot find documentation for the Dir.inject method. I assume from some of the Ruby docs this is like the Smalltalk inject method, but I don't know why it's not documented in the Dir class?

Anyway, what, exactly, is happening in the line (typo removed)

> moved = dir.inject(0) do |count, logfile|

and how do you know to put the | count, logfile | variables in that order, and not | logfile, count | ?

5

Eivind Eklund wrote:

# ... and continue refactoring into an object.

I can do another pass if you finish the refactoring job.

Eivind.

Thanks. Okay, here's refactoring pass 1:

···

=============================================

#!/usr/bin/ruby -w
#########################################################
#
# log_CID.rb
#
# Ruby CID logging script
#
# Logs all activity at a modem set to decipher CID
# (Caller ID) information. Archives the daily logs as well.
#
##########################################################

require 'zip/zip'
require 'serialport/serialport.so'

############################################
#
# local settings here.
#
# name of archive file--daily logs are moved to this archive
backup_zip_filename = "CID_Data.zip"
# modem initialization string.
# need to set to log verbose caller ID information (+VCID=1 or #CID=1, etc)
# also need to set to NOT answer, just monitor line (usually the default)
modem_init_string = "AT+VCID=1\r\n" # for USR verbose CID output
# directory to kep log files
backup_dir = "c:/modemlog"
# base log name for daily log files
# daily log file name is this, plus YYYYMMDD date, plus .log extension
# eg. CID20041004.log
base_log_name = "CID"
# the comm port having the CID-capable modem
port_name = 'COM3:'
# days that a daily log file is kept prior to archiving the file
days_before_archive = 7
# maximum port read errors allowed before aborting run
MAX_PORT_ERRORS = 3000
# debug on or off?
DEBUG = true
#
##############################################

#################################

# script local class

class DailyLogWithArchive
     attr_reader :backup_dir, :base_log_name, :backup_zip_filename,
       :archive_days_secs, :backup_days_interval

     def initialize(backup_dir, base_log_name, backup_zip_filename,
       days_before_backup, backup_days_interval = 1)
         @backup_dir = backup_dir
         @base_log_name = base_log_name
         @backup_zip_filename = backup_zip_filename
         @archive_days_secs = 60 * 60 * 24 * days_before_backup
         @backup_days_interval = backup_days_interval
         # var for calendar based backup, start with invalid value.
         @last_backup_day = -1
     end

     def current_fname
         "#{backup_dir}/#{base_log_name}#{Time.now.strftime("%Y%m%d")}.log"
     end

     def archive_old_to_zip
         time = Time.now
         dir = Dir.open(backup_dir)
         moved = dir.inject(0) do | move_count, logfile |
             next unless logfile.index(base_log_name) == 0
             next unless time > File.stat(logfile).mtime + archive_days_secs
             if(move_to_archive(logfile))
                 next move_count + 1
             else
                 next move_count
             end
         end
         dir.close
         return moved
     end

     def move_to_archive(fname)
         Zip::ZipFile.open(backup_zip_filename, 1) {
             > zfile |
             zfile.add(fname, fname)
         }
         File.delete(fname)
     end

     def log_text(txt)
         logfile = File.new(current_fname, "a")
         logfile.print(txt)
         logfile.close
         # archive old logs daily
         time = Time.now
         yday = time.yday
         if(yday != last_backup_day and
           (yday >= last_backup_day + backup_days_interval or yday == 0) )
             archive_old_to_zip
             last_backup_day = yday
         end
     end

end

###############################
# begin main program

# var to hold port read error count
port_err_count = 0

# move to the dir for backup
Dir.chdir(backup_dir)

# Open the port and set up CID via modem_init_string.
port = SerialPort.new(port_name)
# indefinite wait for a string to appear at the port
port.read_timeout = 0
port.puts(modem_init_string)

print "Starting run with port ", port_name,
   " and logging to dir ", backup_dir, "\n" if DEBUG

# set up the logging class
logger =
   DailyLogWithArchive.new(backup_dir, base_log_name, backup_zip_filename)

# Loop with pauses to look for data at the port we can record.
while(true)
     while(text = port.gets)
         print text if DEBUG
         # log text unless it is just spaces
         if(text =~ /\S/)
             # squeeze double \r, etc to just \n
             text.sub!(/[\r\n]+/, "\n")
             logger.log_text(text)
         end
     end
     msg = "#{Time.now.to_s}: dropped out of system call, restarting loop.\n"
     print msg if DEBUG
     logger.log_text(msg) if DEBUG
     port_err_count += 1
     if(port_err_count > MAX_PORT_ERRORS)
         msg = "Too many port errors...exiting\n"
         print msg
         logger.log_text(msg)
         return port_err_count
     end
end

return 0

===========================================

6

So I'm just hanging out while my wife packs...

     I haven't been following this thread much (sorry) but here's a
style suggestion: instead of doing a bunch of conditional return
true/return false statements, you can make use of the fact that booleans
are first class values and write:
     
     def move_to_archive(fname)
         Zip::ZipFile.open(@archive_zip_filename, 1) {
             > zfile |
             return nil if zfile.exist?(fname)
             zfile.add(fname, fname)
         }
           File.delete(fname) == 1
     end

     def archiving_needed
         # check if we should do periodic archiving (up to daily)
         yday = Time.now.yday
           next_archive_day = @last_archive_day + @archive_days_interval
           yday != @last_archive_day and yday >= next_archive_day
    end

I also note in this one that it does not seem to handle
year-end-wrapping and also that IIRC yday will never be 0. You may want
to look at using the raw Time.now (instead of the yday) and storing your
intervals in seconds.

Sorry that these are so late to the table...

    -- Markus

···

On Sat, 2004-10-16 at 15:24, Bill wrote:

Okay--I think this is the last revision unless there's a feature
request. Thanks to all who helped with the Ruby/Smalltalk idioms.

==========================
#!/usr/bin/ruby -w
#########################################################
#
# log_CID.rb
#
# Ruby CID logging script
#
# Logs all activity at a modem set to decipher CID
# (Caller ID) information. Archives the daily logs as well.
#
##########################################################

require 'zip/zip'
require 'serialport/serialport.so'
require 'yaml'
require 'getoptlong'

#################################

class ModemCIDMonitor
     def initialize(config, logger)
         @port_name = config.port_name
         @MAX_PORT_ERRORS = config.max_port_errors
         @debug = config.debug
         @log_blank_lines = config.log_blank_lines
         @modem_init_string = config.modem_init_string
         @logger = logger
         @port_err_count = 0
         @port = SerialPort.new(@port_name)
         @port.read_timeout = 0
         @port.puts(@modem_init_string)
     end

     def log(txt)
         print txt if @debug
         @logger.log_text(txt)
     end

     def run
         print "Starting run with port ", @port_name,
           " and logging to dir ", @logger.archive_dir, "\n"
         loop do
             @port.each_line do | text |
                 next unless text =~ /\S/ or @log_blank_lines
                     # squeeze double \r, etc to just \n
                     text.sub!(/[\r\n]+/, "\n")
                     log(text)
             end
             msg =
              "#{Time.now.to_s}: dropped out of system call, restarting
loop.\n"
             log(msg) if @debug
             @port_err_count += 1
             if(@port_err_count > MAX_PORT_ERRORS)
                 errmsg = "Too many port errors...ending run\n"
                 log(errmsg)
                 return errmsg
             end
         end
     end
end

class CID_Config
     attr_reader :config_file, :as_hash, :defaults

     def initialize(config_file)
         @config_file = config_file
         @as_hash = YAML::load( File.open(@config_file) )
         @defaults = {
                             'archive_dir' => './',
                             'base_log_name' => 'CID',
                             'debug' => false,
                             'archive_days_interval' => 1,
                             'archive_zip_filename' => 'CID_archive.zip',
                             'port_name' => 'COM1',
                             'max_port_errors' => 100,
                             'log_blank_lines' => false,
                             'modem_init_string' => "AT+VCID=1\r\n",
                         }
         @defaults.each do | k, v |
             @as_hash[k] = v unless @as_hash.has_key?(k)
         end
     end

     def method_missing(method, *args)
         method_key = method.to_s
         if args.length == 0 and @as_hash.has_key?(method_key)
             return @as_hash[method_key]
         else
             super
         end
     end

end

class DailyLogWithArchive
     attr_reader :archive_dir, :base_log_name, :archive_zip_filename,
       :archive_days_secs, :archive_days_interval
     attr_accessor :debug

     def initialize(config)
         @archive_dir = config.archive_dir
         @base_log_name = config.base_log_name
         @debug = config.debug
         @archive_days_interval = config.archive_days_interval
         @archive_zip_filename = config.archive_zip_filename
         @last_archive_day = -1
         @secs_before_archive = 60 * 60 * 24 * config.days_before_archive
     end

     def current_fname
         "#{archive_dir}/#{base_log_name}#{Time.now.strftime('%Y%m%d')}.log"
     end

     def logfile_needs_moving(logfile)
         # check if the logfile mtime is old enough to archive the file
         return false unless logfile.index(@base_log_name) == 0
         return false unless Time.now >
           File.stat(logfile).mtime + @secs_before_archive
         return true
     end

     def archiving_needed
         # check if we should do periodic archiving (up to daily)
         yday = Time.now.yday
         return false unless yday != @last_archive_day
         return false unless
           yday >= @last_archive_day + @archive_days_interval or
           yday == 0
         return true
     end

     def archive_old_to_zip
         moved = 0
         Dir.chdir(@archive_dir) do
             dir = Dir.open('.')
             moved = dir.inject(0) do | move_count, logfile |
                 next unless logfile_needs_moving(logfile)
                 next unless move_to_archive(logfile)
                 log_text("LOGGER: Archiving file " + logfile + "\n") if
@debug
                 next move_count + 1
             end
         end
         return moved
     end

     def move_to_archive(fname)
         Zip::ZipFile.open(@archive_zip_filename, 1) {
             > zfile |
             return nil if zfile.exist?(fname)
             zfile.add(fname, fname)
         }
         rc = File.delete(fname)
         return true if rc and rc == 1
         return nil
     end

     def log_text(txt)
         Dir.chdir(@archive_dir) do
             File.open(current_fname, "a") do | logfile |
                 logfile.print(txt)
             end
             if(archiving_needed)
                 archive_old_to_zip
                 @last_archive_day = Time.now.yday
             end
         end
     end

end

###############################

config_yaml = 'log_CID.yml' # default config file
opts = GetoptLong.new( [ "--conf", "-c", GetoptLong::REQUIRED_ARGUMENT ] )
opts.each do | opt, arg |
     if(opt == "--conf" and arg.length > 0)
         config_yaml = arg
         last
     end
end

config = CID_Config.new(config_yaml)
logger = DailyLogWithArchive.new(config)
monitor = ModemCIDMonitor.new(config, logger)

# no return from run unless abort on port timeouts or errors
errmsg = monitor.run

print errmsg

return 0

==========================

############################################
#
# log_CID YAML config file.
#

# name of archive file--daily logs are moved to this archive
archive_zip_filename: "CID_Data.zip"

# modem initialization string.
# need to set to log verbose caller ID information (+VCID=1 or #CID=1, etc)
# also need to set to NOT answer, just monitor line (usually the default)
modem_init_string: "AT+VCID=1\r\n"

# directory to kep log files
archive_dir: "c:/modemlog"

# base log name for daily log files
# daily log file name is this, plus YYYYMMDD date, plus .log extension
# eg. CID20041004.log
base_log_name: CID

# the comm port having the CID-capable modem
port_name: 'COM1:'

# days that a daily log file is kept prior to archiving the file
days_before_archive: 7

# maximum port read errors allowed before aborting run
max_port_errors: 3000

# whether to log whitespace-only lines
log_blank_lines: false

# debug on or off?
debug: true

#
# end YAML config file.
#
#####################################################

7

Bill wrote:

One problem: I cannot find documentation for the Dir.inject method. I assume from some of the Ruby docs this is like the Smalltalk inject method, but I don't know why it's not documented in the Dir class?

The Dir class includes the Enumerable module (like most classes that has a 'each' method), and that module has the inject method.

Anyway, what, exactly, is happening in the line (typo removed)

> moved = dir.inject(0) do |count, logfile|

and how do you know to put the | count, logfile | variables in that order, and not | logfile, count | ?

Go to the doc for Enumerable#inject and you'll see the description for it.

Robo

8

One problem: I cannot find documentation for the Dir.inject method. I
assume from some of the Ruby docs this is like the Smalltalk inject
method, but I don't know why it's not documented in the Dir class?

     Yes, very much like.
     It is a mix-in, from the module Enumerable; thus anything that has
an each & includes Enumerable (e.g. arrays, files, etc.) supports
inject. Rather than documenting it for each of these most references
just list the "mix-ins" of a class as a reminder that you are getting
some extra goodies for free.

Anyway, what, exactly, is happening in the line (typo removed)

> moved = dir.inject(0) do |count, logfile|

and how do you know to put the | count, logfile | variables in that
order, and not | logfile, count | ?

     Inject works on the model of "injecting" an operator between each
element of an Enumerable collection, prefaced with a starting value.
Thus:

     [2,5,7,4].inject(0) { |running_total,x| running_total + x }

means:

     0 + 2 + 5 + 7 + 4

or (in case order of evaluation matters to you):

     (((0 + 2) + 5) + 7) + 4

just like (IIRC) in smalltalk.

-- Markus

···

On Mon, 2004-10-11 at 22:14, Bill wrote:

9

Thanks. Okay, here's refactoring pass 1:

############################################
#
# local settings here.
#
# name of archive file--daily logs are moved to this archive
backup_zip_filename = "CID_Data.zip"
# modem initialization string.
# need to set to log verbose caller ID information (+VCID=1 or #CID=1, etc)
# also need to set to NOT answer, just monitor line (usually the default)
modem_init_string = "AT+VCID=1\r\n" # for USR verbose CID output
# directory to kep log files
backup_dir = "c:/modemlog"
# base log name for daily log files
# daily log file name is this, plus YYYYMMDD date, plus .log extension
# eg. CID20041004.log
base_log_name = "CID"
# the comm port having the CID-capable modem
port_name = 'COM3:'
# days that a daily log file is kept prior to archiving the file
days_before_archive = 7
# maximum port read errors allowed before aborting run
MAX_PORT_ERRORS = 3000
# debug on or off?
DEBUG = true
#
#
##############################################

All of these "lonely constants and variables" seems ... wrong.
Wouldn't they be better off as methods on an object?

The class refactoring looked OK, except that I would line up the right
hand side of the assignments in initialize.

###############################
# begin main program

# var to hold port read error count
port_err_count = 0

# move to the dir for backup
Dir.chdir(backup_dir)

# Open the port and set up CID via modem_init_string.
port = SerialPort.new(port_name)
# indefinite wait for a string to appear at the port
port.read_timeout = 0
port.puts(modem_init_string)

print "Starting run with port ", port_name,
   " and logging to dir ", backup_dir, "\n" if DEBUG

# set up the logging class
logger =
   DailyLogWithArchive.new(backup_dir, base_log_name, backup_zip_filename)

# Loop with pauses to look for data at the port we can record.
while(true)

Use loop do ... end instead of white(true)

     while(text = port.gets)

Use port.each_line do |text|

         print text if DEBUG
         # log text unless it is just spaces
         if(text =~ /\S/)

I'd turn this on it's head: next if text =~ /^\s*$/

             # squeeze double \r, etc to just \n
             text.sub!(/[\r\n]+/, "\n")
             logger.log_text(text)
         end
     end
     msg = "#{Time.now.to_s}: dropped out of system call, restarting
loop.\n"
     print msg if DEBUG
     logger.log_text(msg) if DEBUG

Join out double conditionals, and move msg calculation under them.

     port_err_count += 1
     if(port_err_count > MAX_PORT_ERRORS)
         msg = "Too many port errors...exiting\n"
         print msg
         logger.log_text(msg)
         return port_err_count
     end
end

return 0

The logger.log_text(msg) / print msg duplication should be joined up
into a single method.

I've got a feeling most of the main loop and constants could, with
benefit, be refactored into a method object.

Eivind.

···

On Wed, 13 Oct 2004 11:04:45 +0900, Bill <wherrera@lynxview.com> wrote:

10

Outgoing mail is certified Virus Free.
Checked by AVG Anti-Virus (http://www.grisoft.com).
Version: 7.0.280 / Virus Database: 264.11.1 - Release Date: 10/15/2004

11

Eivind Eklund wrote:

All of these "lonely constants and variables" seems ... wrong. Wouldn't they be better off as methods on an object?

I guess so. That would, I guess, mean creating a ModemMonitor class (derived from a MonitorServiceClass?), but you'd still need to put the initialization values somewhere where they could be edited for script application on different systems.

Has anyone created a YAML.rb derived class for config files? That would pull those 'lonely' variables out into a separate file?

···

I've got a feeling most of the main loop and constants could, with
benefit, be refactored into a method object.

12

Abe Vionas wrote:

Outgoing mail is certified Virus Free.
Checked by AVG Anti-Virus (http://www.grisoft.com).
Version: 7.0.280 / Virus Database: 264.11.1 - Release Date: 10/15/2004

Where's the message?

heres a site which has a couple describing though
http://careo.elearning.ubc.ca/weblogs/mitch/archives/007113.html

David Ross

···

--
Hazzle free packages for Ruby?
RPA is available from http://www.rubyarchive.org/

13

there's one in here (search for Config). sorry for long post.

save as 'main.rb' and try runnning 'main.rb --template=conf' to generate a
config file for this main. use with 'main.rb --config=conf -v4'

#!/usr/bin/env ruby

···

On Wed, 13 Oct 2004, Bill wrote:

Has anyone created a YAML.rb derived class for config files? That would pull
those 'lonely' variables out into a separate file?

#
# builtin libs
#
   require 'optparse'
   require 'yaml'
   require 'pp'
   require 'pathname'
#
# logging methods
#
   require "logger"
   module Logging
#{{{
   #
   # a module that adds an accessor to Logging objects in ored to fix a bug where
   # not all logging devices are put into sync mode, resulting in improper log
   # rolling. this is a hack.
   #
     module LoggerExt
#{{{
       attr :logdev
#}}}
     end # module LoggerExt
   #
   # implementations of the methods shared by both classes and objects of classes
   # which include Logging
   #
     module LogMethods
#{{{
       def logger
#{{{
         if defined?(@logger) and @logger
           @logger
         else
           if Class === self
             @logger = self.default_logger
           else
             @logger = self::class::logger
           end
           raise "@logger is undefined!" unless defined?(@logger) and @logger
           @logger
         end
#}}}
       end
       def logger= log
#{{{
         @logger = log
         @logger.extend LoggerExt
         @logger.logdev.dev.sync = true
         @logger
#}}}
       end
       def debug(*args, &block); logger.debug(*args, &block); end
       def info(*args, &block); logger.info(*args, &block) ; end
       def warn(*args, &block); logger.warn(*args, &block) ; end
       def error(*args, &block); logger.error(*args, &block); end
       def fatal(*args, &block); logger.fatal(*args, &block); end
       def log_err e
#{{{
         if logger.debug?
           error{ errmsg e }
         else
           error{ emsg e }
         end
#}}}
       end
       def emsg e
#{{{
         "#{ e.message } - (#{ e.class })"
#}}}
       end
       def btrace e
#{{{
         e.backtrace.join("\n")
#}}}
       end
       def errmsg e
#{{{
         emsg(e) << "\n" << btrace(e)
#}}}
       end
#}}}
     end # module LogMethods
     EOL = "\n"
     DIV0 = ("." * 79) << EOL
     DIV1 = ("-" * 79) << EOL
     DIV2 = ("=" * 79) << EOL
     DIV3 = ("#" * 79) << EOL
     SEC0 = ("." * 16) << EOL
     SEC1 = ("-" * 16) << EOL
     SEC2 = ("=" * 16) << EOL
     SEC3 = ("#" * 16) << EOL
     class << self
#{{{
       def append_features c
#{{{
         ret = super
         c.extend LogMethods
         class << c
           def default_logger
#{{{
             if defined?(@default_logger) and @default_logger
               @default_logger
             else
               self.default_logger = Logger::new STDOUT
               @default_logger.debug{ "<#{ self }> using default logger"}
               @default_logger
             end
#}}}
           end
           def default_logger= log
#{{{
             @default_logger = log
             @default_logger.extend LoggerExt
             @default_logger.logdev.dev.sync = true
             @default_logger
#}}}
           end
         end
         ret
#}}}
       end
#}}}
     end
     include LogMethods
#}}}
   end # module Logging
#
# utility methods
#
   require 'pathname'
   require 'socket'
   require 'tmpdir'
   module Util
#{{{
     class << self
       def export sym
#{{{
         sym = "#{ sym }".intern
         module_function sym
         public sym #}}}
       end
       def append_features c
#{{{
         super
         c.extend Util #}}}
       end
     end
     def mcp obj
#{{{
       Marshal.load(Marshal.dump(obj))
#}}}
     end
     export 'mcp'
     def klass
#{{{
       self.class
#}}}
     end
     export 'klass'
     def realpath path
#{{{
       path = File::expand_path "#{ path }"
       begin
         Pathname::new(path).realpath.to_s
       rescue Errno::ENOENT, Errno::ENOTDIR
         path
       end
#}}}
     end
     export 'realpath'
     def hashify(*hashes)
#{{{
       hashes.inject(accum={}){|accum,hash| accum.update hash}
#}}}
     end
     export 'hashify'
     def getopt opt, hash
#{{{
       opt_s = "#{ opt }"
       hash[opt] || hash[opt_s] || hash[opt_s.intern] #}}}
     end
     export 'getopt'
     def alive? pid
#{{{
       pid = Integer("#{ pid }")
       begin
         Process.kill 0, pid
         true
       rescue Errno::ESRCH
         false
       end
#}}}
     end
     export 'alive?'
     def maim(pid, opts = {})
#{{{
       sigs = getopt('signals', opts) || %w(SIGTERM SIGQUIT SIGKILL)
       suspend = getopt('suspend', opts) || 4
       pid = Integer("#{ pid }")
       sigs.each do |sig|
         begin
           Process.kill(sig, pid)
         rescue Errno::ESRCH
           return nil
         end
         sleep 0.2
         unless alive?(pid)
           break
         else
           sleep suspend
         end
       end
       not alive?(pid)
#}}}
     end
     export 'maim'
     def timestamp time = Time.now
#{{{
       usec = "#{ time.usec }"
       usec << ('0' * (6 - usec.size)) if usec.size < 6
       time.strftime('%Y-%m-%d %H:%M:%S.') << usec
#}}}
     end
     export 'timestamp'
     def stamptime string, local = true #{{{
       string = "#{ string }"
       pat = %r/^\s*(\d\d\d\d)-(\d\d)-(\d\d) (\d\d):(\d\d):(\d\d).(\d\d\d\d\d\d)\s*$/o
       match = pat.match string
       raise ArgumentError, "<#{ string.inspect }>" unless match
       yyyy,mm,dd,h,m,s,u = match.to_a[1..-1].map{|m| m.to_i}
       if local
         Time.local yyyy,mm,dd,h,m,s,u
       else
         Time.gm yyyy,mm,dd,h,m,s,u
       end
#}}}
     end
     export 'stamptime'
     def escape! s, char, esc
#{{{
       re = %r/([#{0x5c.chr << esc}]*)#{char}/
       s.gsub!(re) do
         (($1.size % 2 == 0) ? ($1 << esc) : $1) + char
       end
#}}}
     end
     export 'escape!'
     def escape s, char, esc
#{{{
       ss = "#{ s }"
       escape! ss, char, esc
       ss
#}}}
     end
     export 'escape'
     def fork(*args, &block)
#{{{
       begin
         verbose = $VERBOSE
         $VERBOSE = nil
         Process.fork(*args, &block)
       ensure
         $VERBOSE = verbose
       end
#}}}
     end
     export 'fork'
     def exec(*args, &block)
#{{{
       begin
         verbose = $VERBOSE
         $VERBOSE = nil
         Kernel.exec(*args, &block)
       ensure
         $VERBOSE = verbose
       end
#}}}
     end
     export 'exec'
     def hostname
#{{{
       @__hostname__ ||= Socket::gethostname
#}}}
     end
     export 'hostname'
     def host
#{{{
       @__host__ ||= Socket::gethostname.gsub(%r/\..*$/o,'')
#}}}
     end
     export 'host'
     def emsg e
#{{{
       "#{ e.message } - (#{ e.class })"
#}}}
     end
     export 'emsg'
     def btrace e
#{{{
       (e.backtrace or ).join("\n")
#}}}
     end
     export 'btrace'
     def errmsg e
#{{{
       emsg(e) << "\n" << btrace(e)
#}}}
     end
     export 'errmsg'
     def erreq a, b
#{{{
       a.class == b.class and
       a.message == b.message and
       a.backtrace == b.backtrace
#}}}
     end
     export 'erreq'
     def tmpnam dir = Dir.tmpdir, seed = File::basename($0)
#{{{
       pid = Process.pid
       path = "%s_%s_%s_%s_%d" %
         [Util::hostname, seed, pid, Util::timestamp.gsub(/\s+/o,'_'), rand(101010)]
       File::join(dir, path)
#}}}
     end
     export 'tmpnam'
     def uncache file #{{{
       refresh = nil
       begin
         is_a_file = File === file
         path = (is_a_file ? file.path : file.to_s)
         stat = (is_a_file ? file.stat : File::stat(file.to_s))
         refresh = tmpnam(File::dirname(path))
         File::link path, refresh rescue File::symlink path, refresh
         File::chmod stat.mode, path
         File::utime stat.atime, stat.mtime, path
       ensure
         begin
           File::unlink refresh if refresh
         rescue Errno::ENOENT
         end
       end
#}}}
     end
     export 'uncache'
#}}}
   end # class Util
# main program class
#
   class Main
#{{{
     include Logging
     include Util

     VERSION = '0.0.0'
     PROGNAM = File::basename(Util::realpath($0))
     CONFIG_DEFAULT_PATH = "#{ PROGNAM }.conf"
     CONFIG_SEARCH_PATH = %w(. ~ /usr/local/etc /usr/etc /etc)

     USAGE = #{{{
<<-usage
NAME
   #{ PROGNAM } v#{ VERSION }

SYNOPSIS
   #{ PROGNAM } [options]+ [file]+

DESCRIPTTION

ENVIRONMENT

CONFIG
   default path => #{ CONFIG_DEFAULT_PATH }
   search path => #{ CONFIG_SEARCH_PATH.inspect }

DIAGNOSTICS
   success => $? == 0
   failure => $? != 0

AUTHOR
   ara.t.howard@noaa.gov

BUGS
   > 1

OPTIONS
usage
#}}}

     OPTSPEC = [
#{{{
       [
         '--help', '-h',
         'this message'
       ],
       [
         '--verbosity=verbostiy', '-v',
         '0|fatal < 1|error < 2|warn < 3|info < 4|debug - (default info)'
       ],
       [
         '--log=path','-l',
         'set log file - (default stderr)'
       ],
       [
         '--log_age=log_age',
         'daily | weekly | monthly - what age will cause log rolling (default nil)'
       ],
       [
         '--log_size=log_size',
         'size in bytes - what size will cause log rolling (default nil)'
       ],
       [
         '--config=path',
         'valid path - specify config file (default nil)'
       ],
       [
         '--template=[path]',
         'valid path - generate a template config file in path (default stdout)'
       ],
     ]
#}}}

     EXAMPLES =
#{{{
<<-examples
EXAMPLES

   0) #{ PROGNAM }
examples
#}}}

     EXIT_SUCCESS = 0
     EXIT_FAILURE = 1

     class Config < Hash
#{{{
       class << self
         def gen_template(arg = nil)
#{{{
           @data ||= DATA.read
           case arg
             when IO
               arg.write @data
             when String
               open(arg, 'w'){|f| f.write @data}
             else
               STDOUT.write @data
           end
           self
#}}}
         end
         def load_default
#{{{
           @data ||= DATA.read
           @default ||= YAML::load(munge(@data)) || {}
#}}}
         end
         def any(basename, *dirnames)
#{{{
           config = nil
           dirnames.flatten.each do |dirname|
             path = File::join dirname, basename
             if test ?e, path
               config = Config::new(path)
               break
             end
           end
           config || Config::new('default') #}}}
         end
         def munge buf
#{{{
           buf.gsub(%r/\t/o,' ')
#}}}
         end
       end
       attr :path
       def initialize path
#{{{
         @path = nil
         yaml = nil
         if path.nil? or path and path =~ /^\s*default/io
           yaml = self.class.load_default
           @path = 'DEFAULT'
         else path
           yaml = YAML::load(self.class.munge(open(path).read))
           @path = path
         end
         self.update yaml
#}}}
       end
       def to_hash
#{{{
         {}.update self
#}}}
       end
#}}}
     end

     attr :logger
     attr :argv
     attr :env
     attr :options
     attr :op
     attr :logdev
     attr :verbosity
     attr :config

     def initialize argv = ARGV, env = ENV
#{{{
       begin
         @logger = Logger::new STDERR
         @argv = Util::mcp(argv.to_a)
         @env = Util::mcp(env.to_hash)
         parse_options
         if @options.has_key? 'help'
           usage STDOUT
           return EXIT_SUCCESS
         end
         if @options.has_key? 'template'
           gen_template @options['template']
           return EXIT_SUCCESS
         end
         parse_argv
         init_logging
         init_config
         status = run
         exit status
       rescue => e
         fatal{ e }
         exit EXIT_FAILURE
       end
#}}}
     end
     def parse_options
#{{{
       @op = OptionParser::new
       @options = {}
       OPTSPEC.each do |spec|
         k = spec.first.gsub(%r/(?:--)|(?:=.*$)|(?:\s+)/o,'')
         @op.def_option(*spec){|v| @options[k] = v}
       end
       #begin
         op.parse! @argv
       #rescue OptionParser::InvalidOption => e
         # preverve unknown options
         #e.recover(argv)
       #end
       @options
#}}}
     end
     def parse_argv
#{{{
       # arg0, arg1 = @argv
#}}}
     end
     def usage io = STDERR
#{{{
       io << USAGE if defined? USAGE
       if defined? OPTSPEC
         OPTSPEC.each do |os|
           a, b, c = os
           long, short, desc = nil
           [a,b,c].each do |word|
             next unless word
             word.strip!
             case word
               when %r/^--/o
                 long = word
               when %r/^-/o
                 short = word
               else
                 desc = word
             end
           end
           spec = ((long and short) ? [long, short] : [long])
           io << " #{ spec.join(', ') }\n"
           io << " #{ desc }\n" if desc
         end
         io << "\n"
       end
       io << EXAMPLES << "\n" if defined? EXAMPLES
       self
#}}}
     end
     def init_logging
#{{{
       log, log_age, log_size, verbosity =
         @options.values_at 'log', 'log_age', 'log_size', 'verbosity'
       log_age = atoi log_age rescue nil
       log_size = atoi log_size rescue nil
       $logger = @logger = Logger::new(log || STDERR, log_age, log_size)
     #
     # hack to fix Logger sync bug
     #
       class << @logger; attr :logdev; end
       @logdev = @logger.logdev.dev
       @logdev.sync = true
       level = nil
       verbosity ||= 'info'
       verbosity =
         case verbosity
           when /^\s*(?:4|d|debug)\s*$/io
             level = 'Logging::DEBUG'
             4
           when /^\s*(?:3|i|info)\s*$/io
             level = 'Logging::INFO'
             3
           when /^\s*(?:2|w|warn)\s*$/io
             level = 'Logging::WARN'
             2
           when /^\s*(?:1|e|error)\s*$/io
             level = 'Logging::ERROR'
             1
           when /^\s*(?:0|f|fatal)\s*$/io
             level = 'Logging::FATAL'
             0
           else
             abort "illegal verbosity setting <#{ verbosity }>"
         end
       @logger.level = 2 - ((verbosity % 5) - 2)
       debug {"logging level <#{ level }>"}
       @logger
#}}}
     end
     def init_config
#{{{
       @config =
         if @options['config']
           Config::new(@options['config'])
         else
           Config::any CONFIG_DEFAULT_PATH, CONFIG_SEARCH_PATH
         end
       debug { "config.path <#{ @config.path }>" }
       debug { "config\n#{ @config.to_hash.to_yaml }\n" }
#}}}
     end
     def gen_template template
#{{{
       Config::gen_template(template)
       self
#}}}
     end
     def run
#{{{
       warn{ "foobar" }
       p 42
       return EXIT_SUCCESS
#}}}
     end
#}}}
   end
#
# run main program unless included as lib
#
   Main::new ARGV, ENV if $0 == __FILE__
#
# the default config is stored here
#
__END__
#
# default config
#

key : value
k2 : v2

-a
--

EMAIL :: Ara [dot] T [dot] Howard [at] noaa [dot] gov
PHONE :: 303.497.6469
When you do something, you should burn yourself completely, like a good
bonfire, leaving no trace of yourself. --Shunryu Suzuki

===============================================================================

14

I've done this before by just putting the variables in a single Hash,
with a gating object around it, then I used YAML to load / save the
hash. That worked well for me.

Eivind.

···

On Thu, 14 Oct 2004 13:29:32 +0900, Bill <bi11@lynxview.com> wrote:

Eivind Eklund wrote:
>
> All of these "lonely constants and variables" seems ... wrong.
> Wouldn't they be better off as methods on an object?

I guess so. That would, I guess, mean creating a ModemMonitor class
(derived from a MonitorServiceClass?), but you'd still need to put the
initialization values somewhere where they could be edited for script
application on different systems.

Has anyone created a YAML.rb derived class for config files? That would
pull those 'lonely' variables out into a separate file?

15

Here's the (nearly last) refactoring:
(text should word wrap at 80, but the news program wants to wrap earlier---)

#!/usr/bin/ruby -w

···

#########################################################
#
# log_CID.rb
#
# Ruby CID logging script
#
# Logs all activity at a modem set to decipher CID
# (Caller ID) information. Archives the daily logs as well.
#
##########################################################

require 'zip/zip'
require 'serialport/serialport.so'
require 'yaml'

# name of config file (YAML format)
config_yaml = 'log_CID.yml'

#################################

# script local classes

class ModemCIDMonitor
     attr_reader :port_err_count, :config, :log_blank_lines
     attr :debug, :port_err_count

     def initialize(config_hash, logger)
         @port_err_count = config_hash['port_err_count'] || 0
         @port_name = config_hash['port_name'] || 'COM1:'
         @port = SerialPort.new(@port_name)
         @MAX_PORT_ERRORS = config_hash['MAX_PORT_ERRORS'] || 100
         @debug = config_hash['DEBUG'] || false
         @log_blank_lines = config_hash['log_blank_lines'] || false
         @logger = logger
         @port.read_timeout = 0
         @port_err_count = 0
         @modem_init_string =
           config_hash['modem_init_string'] || "AT+VCID=1\r\n"
         @port.puts(@modem_init_string)
     end

     def log(txt)
         print txt if debug
         @logger.log_text(txt)
     end

     def run
         print "Starting run with port ", @port_name,
           " and logging to dir ", @logger.archive_dir, "\n"
         loop do
             @port.each_line do | text |
                 # log text unless it is just spaces
                 next unless text =~ /\S/ or log_blank_lines
                     # squeeze double \r, etc to just \n
                     text.sub!(/[\r\n]+/, "\n")
                     log(text)
             end
             msg =
              "#{Time.now.to_s}: dropped out of system call, restarting loop.\n"
             log(msg) if debug
             @port_err_count += 1
             if(@port_err_count > MAX_PORT_ERRORS)
                 errmsg = "Too many port errors...ending run\n"
                 log(errmsg)
                 return errmsg
             end
         end
     end
end

class CID_Config
     attr_reader :config_file, :as_hash

     def initialize(config_file)
         @config_file = config_file
         @as_hash = YAML::load( File.open(@config_file))
     end
end

class DailyLogWithArchive
     attr_reader :archive_dir, :base_log_name, :archive_zip_filename,
       :archive_days_secs, :archive_days_interval
     attr :debug

     def initialize(config_hash)
         @archive_dir = config_hash['archive_dir'] >> './'
         @base_log_name = config_hash['base_log_name'] >> 'CID'
         @debug = config_hash['DEBUG'] >> false
         @archive_days_interval = config_hash['archive_days_interval'] >> 1
         @archive_zip_filename = config_hash['archive_zip_filename'] ||
                                     'CID_archive.zip'
         @last_archive_day = -1
         @archive_days_secs = 60 * 60 * 24 * 7
         @archive_days_secs =
           60 * 60 * 24 * config_hash['days_before_archive'] if
             (config_hash['days_before_archive'])
     end

     def current_fname
         "#{archive_dir}/#{base_log_name}#{Time.now.strftime("%Y%m%d")}.log"
     end

     def archive_old_to_zip
         time = Time.now
         wd = Dir.getwd
         Dir.chdir(@archive_dir)
         dir = Dir.open(@archive_dir)
         moved = dir.inject(0) do | move_count, logfile |
             next unless logfile
             next unless logfile =~ /^#{base_log_name}/
             next unless time > File.stat(logfile).mtime + @archive_days_secs
             if(move_to_archive(logfile))
                 log_text("LOGGER: Archiving file " + logfile + "\n") if @debug
                 next move_count + 1
             else
                 next move_count
             end
         end
         dir.close
         Dir.chdir(wd)
         return moved
     end

     def move_to_archive(fname)
         Zip::ZipFile.open(@archive_zip_filename, 1) {
             > zfile |
             return nil if zfile.exist?(fname)
             zfile.add(fname, fname)
         }
         File.delete(fname)
     end

     def log_text(txt)
         wd = Dir.getwd
         Dir.chdir(@archive_dir)
         logfile = File.new(current_fname, "a")
         logfile.print(txt)
         logfile.close
         # check if we should do periodic archiving (up to daily)
         time = Time.now
         yday = time.yday
         if(yday != @last_archive_day and
           (yday >= @last_archive_day + @archive_days_interval or yday == 0) )
             archive_old_to_zip
             @last_archive_day = yday
         end
         Dir.chdir(wd)
     end
end

###############################
# begin main program

# get config from YAML text file
config = CID_Config.new(config_yaml)

# set up the logging class
logger = DailyLogWithArchive.new(config.as_hash)

# set up modem monitor
monitor = ModemCIDMonitor.new(config.as_hash, logger)

# run -- no return from this unless abort on port timeouts or errors
errmsg = monitor.run

print errmsg

return 0

=================================================

log_CID.yml file:

############################################
#
# log_CID YAML config file.
#

# name of archive file--daily logs are moved to this archive
archive_zip_filename: CID_Data.zip

# modem initialization string.
# need to set to log verbose caller ID information (+VCID=1 or #CID=1, etc)
# also need to set to NOT answer, just monitor line (usually the default)
modem_init_string: AT+VCID=1\r\n

# directory to keep log files
archive_dir: c:/USR/log_CID

# base log name for daily log files
# daily log file name is this, plus YYYYMMDD date, plus .log extension
# eg. CID20041004.log
base_log_name: CID

# the comm port having the CID-capable modem
port_name: 'COM3:'

# days that a daily log file is kept prior to archiving the file
days_before_archive: 7

# maximum port read errors allowed before aborting run
MAX_PORT_ERRORS: 3000

# whether to log whitespace-only lines
log_blank_lines: false

# debug on or off?
DEBUG: true

#
# end YAML config file.
#
#####################################################

16

Here's the (nearly last) refactoring:
(text should word wrap at 80, but the news program wants to wrap earlier---)

This is starting to look good :slight_smile: I have some more things, but now
the code is starting to look all right and I'm giving you "polish".

The primary comment I have is that you are passing around hashes for
configuration, instead of having methods on the configuration class.
Make the configuration class pull its weight. I'll show you how
below.

Many of the design cleanups I'm doing here are described in
http://rpa-base.rubyforge.org/wiki/wiki.cgi?GoodAPIDesign

I've tried to collect/organize much of the day-to-day design/craft stuff there.

#!/usr/bin/ruby -w
#########################################################
#
# log_CID.rb
#
# Ruby CID logging script
#
# Logs all activity at a modem set to decipher CID
# (Caller ID) information. Archives the daily logs as well.
#
##########################################################

require 'zip/zip'
require 'serialport/serialport.so'
require 'yaml'

# name of config file (YAML format)
config_yaml = 'log_CID.yml'

I would take this in as command line parameter, possibly using a
default if none was supplied.

#################################

# script local classes

This comment really repeats the code.

class ModemCIDMonitor
     attr_reader :port_err_count, :config, :log_blank_lines
     attr :debug, :port_err_count

     def initialize(config_hash, logger)
         @port_err_count = config_hash['port_err_count'] || 0
         @port_name = config_hash['port_name'] ||
'COM1:'
         @port = SerialPort.new(@port_name)
         @MAX_PORT_ERRORS = config_hash['MAX_PORT_ERRORS'] || 100
         @debug = config_hash['DEBUG'] ||
false
         @log_blank_lines = config_hash['log_blank_lines'] ||
false
         @logger = logger
         @port.read_timeout = 0
         @port_err_count = 0
         @modem_init_string =
           config_hash['modem_init_string'] || "AT+VCID=1\r\n"
         @port.puts(@modem_init_string)
     end

All of these defaults can be relocated to the config class (or
possibly a specific config class for ModemCIDMonitor, but I wouldn't
bother.)

     def log(txt)
         print txt if debug
         @logger.log_text(txt)
     end

     def run
         print "Starting run with port ", @port_name,
           " and logging to dir ", @logger.archive_dir, "\n"
         loop do
             @port.each_line do | text |
                 # log text unless it is just spaces
                 next unless text =~ /\S/ or log_blank_lines
                     # squeeze double \r, etc to just \n
                     text.sub!(/[\r\n]+/, "\n")
                     log(text)
             end
             msg =
              "#{Time.now.to_s}: dropped out of system call, restarting
loop.\n"
             log(msg) if debug
             @port_err_count += 1
             if(@port_err_count > MAX_PORT_ERRORS)
                 errmsg = "Too many port errors...ending run\n"
                 log(errmsg)
                 return errmsg
             end
         end
     end
end

class CID_Config
     attr_reader :config_file, :as_hash

     def initialize(config_file)
         @config_file = config_file
         @as_hash = YAML::load( File.open(@config_file))
     end
end

Here, you're generally exposing the fact that YAML loads a hash.
Don't. Instead, add something like

def method_missing(:symbol, *args)
    symbol = symbol.to_s
    if args.length == 0 && @as_hash.has_key?.symbol
        @as_hash[symbol]
    else
        super
    end
end

Now you can intermix normal methods and stuff that come from the hash;
the hash has become an implementation detail of CID_Config.

class DailyLogWithArchive
     attr_reader :archive_dir, :base_log_name, :archive_zip_filename,
       :archive_days_secs, :archive_days_interval
     attr :debug

     def initialize(config_hash)
         @archive_dir = config_hash['archive_dir']
  >> './'
         @base_log_name = config_hash['base_log_name']
  >> 'CID'
         @debug = config_hash['DEBUG']
  >> false
         @archive_days_interval = config_hash['archive_days_interval']
  >> 1
         @archive_zip_filename = config_hash['archive_zip_filename'] ||
                                     'CID_archive.zip'
         @last_archive_day = -1
         @archive_days_secs = 60 * 60 * 24 * 7
         @archive_days_secs =
           60 * 60 * 24 * config_hash['days_before_archive'] if
             (config_hash['days_before_archive'])
     end

These defaults can be moved to the config class.
class CID_Config
     def days_before_archive
           @as_hash['days_before_archive'] || 7
     end
end

Then your initialization becomes
     @secs_before_archive = 60*60*24*config.days_before_archive

I like this name better than archive_days_secs, because it
shows the relation between the different values much better.
I've added this as an example to the "Normalize your naming"
section of GoodAPIDesign.

     def current_fname
         "#{archive_dir}/#{base_log_name}#{Time.now.strftime("%Y%m%d")}.log"
     end

     def archive_old_to_zip
         time = Time.now
         wd = Dir.getwd
         Dir.chdir(@archive_dir)

Use Dir.chdir(@archive_dir) do ... end

         dir = Dir.open(@archive_dir)

Use "." instead of @archive_dir, to avoid being explict about the
current directory.

         moved = dir.inject(0) do | move_count, logfile |
             next unless logfile

Can this happen? I shouldn't think it can?

             next unless logfile =~ /^#{base_log_name}/
             next unless time > File.stat(logfile).mtime +
@archive_days_secs

These two pieces of logic are important enough that I might have
chosen to move them out to a separate method.

             if(move_to_archive(logfile))
                 log_text("LOGGER: Archiving file " + logfile + "\n") if
@debug
                 next move_count + 1
             else
                 next move_count
             end
         end
         dir.close
         Dir.chdir(wd)
         return moved
     end

     def move_to_archive(fname)
         Zip::ZipFile.open(@archive_zip_filename, 1) {
             > zfile |
             return nil if zfile.exist?(fname)
             zfile.add(fname, fname)
         }
         File.delete(fname)
     end

     def log_text(txt)
         wd = Dir.getwd
         Dir.chdir(@archive_dir)

Use
   Dir.chdir(@archive_dir) do
     ...
   end
instead of messing about with wd. chdir can save the state for you.

         logfile = File.new(current_fname, "a")
         logfile.print(txt)
         logfile.close

Use
    File.new(current_fname, "a") do |logfile|
         logfile.print(txt)
    end
instead of doing the close manually.

         # check if we should do periodic archiving (up to daily)
         time = Time.now
         yday = time.yday

Lose the temporary time variable.

         if(yday != @last_archive_day and
           (yday >= @last_archive_day + @archive_days_interval or yday
== 0) )
             archive_old_to_zip
             @last_archive_day = yday
         end
         Dir.chdir(wd)
     end
end

###############################
# begin main program

Redundant comment.

# get config from YAML text file
config = CID_Config.new(config_yaml)

# set up the logging class
logger = DailyLogWithArchive.new(config.as_hash)

# set up modem monitor
monitor = ModemCIDMonitor.new(config.as_hash, logger)

The above comments just repeat the code. I'd lose the comments and
the extra blank lines (but the blank lines are closer to a personal
preference thing.)

# run -- no return from this unless abort on port timeouts or errors
errmsg = monitor.run

print errmsg

return 0

I'd lose the extra blank lines here, too.

Eivind.

···

On Fri, 15 Oct 2004 13:59:32 +0900, Bill <wherrera@lynxview.com> wrote:
--
Hazzle free packages for Ruby?
RPA is available from http://www.rubyarchive.org/

17

Eivind Eklund wrote:

Here's the (nearly last) refactoring:
(text should word wrap at 80, but the news program wants to wrap earlier---)

This is starting to look good :slight_smile: I have some more things, but now
the code is starting to look all right and I'm giving you "polish".

The primary comment I have is that you are passing around hashes for
configuration, instead of having methods on the configuration class. Make the configuration class pull its weight. I'll show you how
below.

Many of the design cleanups I'm doing here are described in
http://rpa-base.rubyforge.org/wiki/wiki.cgi?GoodAPIDesign

I've tried to collect/organize much of the day-to-day design/craft stuff there.

#!/usr/bin/ruby -w
#########################################################
#
# log_CID.rb
#
# Ruby CID logging script
#
# Logs all activity at a modem set to decipher CID
# (Caller ID) information. Archives the daily logs as well.
#
##########################################################

require 'zip/zip'
require 'serialport/serialport.so'
require 'yaml'

# name of config file (YAML format)
config_yaml = 'log_CID.yml'

I would take this in as command line parameter, possibly using a
default if none was supplied.

#################################

# script local classes

This comment really repeats the code.

class ModemCIDMonitor
    attr_reader :port_err_count, :config, :log_blank_lines
    attr :debug, :port_err_count

    def initialize(config_hash, logger)
        @port_err_count = config_hash['port_err_count'] || 0
        @port_name = config_hash['port_name'] ||
'COM1:'
        @port = SerialPort.new(@port_name)
        @MAX_PORT_ERRORS = config_hash['MAX_PORT_ERRORS'] || 100
        @debug = config_hash['DEBUG'] ||
false
        @log_blank_lines = config_hash['log_blank_lines'] ||
false
        @logger = logger
        @port.read_timeout = 0
        @port_err_count = 0
        @modem_init_string =
          config_hash['modem_init_string'] || "AT+VCID=1\r\n"
        @port.puts(@modem_init_string)
    end

All of these defaults can be relocated to the config class (or
possibly a specific config class for ModemCIDMonitor, but I wouldn't
bother.)

    def log(txt)
        print txt if debug
        @logger.log_text(txt)
    end

    def run
        print "Starting run with port ", @port_name,
          " and logging to dir ", @logger.archive_dir, "\n"
        loop do
            @port.each_line do | text |
                # log text unless it is just spaces
                next unless text =~ /\S/ or log_blank_lines
                    # squeeze double \r, etc to just \n
                    text.sub!(/[\r\n]+/, "\n")
                    log(text)
            end
            msg =
             "#{Time.now.to_s}: dropped out of system call, restarting
loop.\n"
            log(msg) if debug
            @port_err_count += 1
            if(@port_err_count > MAX_PORT_ERRORS)
                errmsg = "Too many port errors...ending run\n"
                log(errmsg)
                return errmsg
            end
        end
    end
end

class CID_Config
    attr_reader :config_file, :as_hash

    def initialize(config_file)
        @config_file = config_file
        @as_hash = YAML::load( File.open(@config_file))
    end
end

Here, you're generally exposing the fact that YAML loads a hash. Don't. Instead, add something like

def method_missing(:symbol, *args)
    symbol = symbol.to_s
    if args.length == 0 && @as_hash.has_key?.symbol
        @as_hash[symbol]
    else
        super
    end
end

I think then, you want something like this?

class CID_Config
     attr_reader :config_file, :as_hash

     def initialize(config_file)
         @config_file = config_file
         @as_hash = YAML::load( File.open(@config_file))
         @defaults = {
                             archive_dir => './'
                             base_log_name => 'CID'
                             DEBUG => false
                             archive_days_interval => 1
                             archive_zip_filename => 'CID_archive.zip'
                             port_name => 'COM1'
                             MAX_PORT_ERRORS => 100
                             log_blank_lines => false
                             modem_init_string => "AT+VCID=1\r\n"
                         }
         defaults.each do | k |
             next if @as_hash.has_key?(k)
             @as_hash[k] = defaults[k]
         end
     end

     def method_missing(:symbol, *args)
         symbol = symbol.to_s
         if args.length == 0 && @as_hash.has_key?.symbol
             @as_hash[symbol]
         else
             super
         end
     end
end

--I don't understand what the super is for here, though?

And, is there any diffrerence between

@my_hash.has_key?.symbol and @my_hash.has_key?{symbol) ?

···

On Fri, 15 Oct 2004 13:59:32 +0900, Bill <wherrera@lynxview.com> wrote:

Now you can intermix normal methods and stuff that come from the hash;
the hash has become an implementation detail of CID_Config.

class DailyLogWithArchive
    attr_reader :archive_dir, :base_log_name, :archive_zip_filename,
      :archive_days_secs, :archive_days_interval
    attr :debug

    def initialize(config_hash)
        @archive_dir = config_hash['archive_dir']
>> './'
        @base_log_name = config_hash['base_log_name']
>> 'CID'
        @debug = config_hash['DEBUG']
>> false
        @archive_days_interval = config_hash['archive_days_interval']
>> 1
        @archive_zip_filename = config_hash['archive_zip_filename'] ||
                                    'CID_archive.zip'
        @last_archive_day = -1
        @archive_days_secs = 60 * 60 * 24 * 7
        @archive_days_secs =
          60 * 60 * 24 * config_hash['days_before_archive'] if
            (config_hash['days_before_archive'])
    end

These defaults can be moved to the config class.
class CID_Config
     def days_before_archive
           @as_hash['days_before_archive'] || 7
     end
end

Then your initialization becomes
     @secs_before_archive = 60*60*24*config.days_before_archive

I like this name better than archive_days_secs, because it
shows the relation between the different values much better.
I've added this as an example to the "Normalize your naming" section of GoodAPIDesign.

    def current_fname
        "#{archive_dir}/#{base_log_name}#{Time.now.strftime("%Y%m%d")}.log"
    end

    def archive_old_to_zip
        time = Time.now
        wd = Dir.getwd
        Dir.chdir(@archive_dir)

Use Dir.chdir(@archive_dir) do ... end

        dir = Dir.open(@archive_dir)

Use "." instead of @archive_dir, to avoid being explict about the
current directory.

        moved = dir.inject(0) do | move_count, logfile |
            next unless logfile

Can this happen? I shouldn't think it can?

            next unless logfile =~ /^#{base_log_name}/
            next unless time > File.stat(logfile).mtime +
@archive_days_secs

These two pieces of logic are important enough that I might have
chosen to move them out to a separate method.

            if(move_to_archive(logfile))
                log_text("LOGGER: Archiving file " + logfile + "\n") if
@debug
                next move_count + 1
            else
                next move_count
            end
        end
        dir.close
        Dir.chdir(wd)
        return moved
    end

    def move_to_archive(fname)
        Zip::ZipFile.open(@archive_zip_filename, 1) {
            > zfile |
            return nil if zfile.exist?(fname)
            zfile.add(fname, fname)
        }
        File.delete(fname)
    end

    def log_text(txt)
        wd = Dir.getwd
        Dir.chdir(@archive_dir)

Use
   Dir.chdir(@archive_dir) do
     ...
   end
instead of messing about with wd. chdir can save the state for you.

        logfile = File.new(current_fname, "a")
        logfile.print(txt)
        logfile.close

Use
    File.new(current_fname, "a") do |logfile|
         logfile.print(txt)
    end
instead of doing the close manually.

        # check if we should do periodic archiving (up to daily)
        time = Time.now
        yday = time.yday

Lose the temporary time variable.

        if(yday != @last_archive_day and
          (yday >= @last_archive_day + @archive_days_interval or yday
== 0) )
            archive_old_to_zip
            @last_archive_day = yday
        end
        Dir.chdir(wd)
    end
end

###############################
# begin main program

Redundant comment.

# get config from YAML text file
config = CID_Config.new(config_yaml)

# set up the logging class
logger = DailyLogWithArchive.new(config.as_hash)

# set up modem monitor
monitor = ModemCIDMonitor.new(config.as_hash, logger)

The above comments just repeat the code. I'd lose the comments and
the extra blank lines (but the blank lines are closer to a personal
preference thing.)

# run -- no return from this unless abort on port timeouts or errors
errmsg = monitor.run

print errmsg

return 0

I'd lose the extra blank lines here, too.

Eivind.