From: Bill Date: 2004-10-16T11:14:30+09:00 Subject: Re: Ruby programming styles and new program? Eivind Eklund wrote: > On Fri, 15 Oct 2004 13:59:32 +0900, Bill 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 > 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) ? > 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.