From: Eivind Eklund <eeklund@...>
Date: 2004-10-13T23:29:57+09:00
Subject: Re: Ruby programming styles and new program?

On Wed, 13 Oct 2004 11:04:45 +0900, Bill <wherrera@lynxview.com> wrote:
> 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.