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