Cleanup and unittest Object, Log, Reporter and ReporterMany

src/main/perl/FileWriter.pm

@@ -164,7 +164,6 @@ sub close
     my $self = shift;
     my ($str, $ret, $cmd, $diff);
 
-
     # We have to do this because Text::Diff is not present in SL5. :(
     if (*$self->{LOG} && $CAF::Reporter::_REP_SETUP->{VERBOSE}
         && -e *$self->{filename} && *$self->{buf}) {
@@ -173,8 +172,8 @@ sub close
                                   keeps_state => 1);
         $cmd->execute();
         *$self->{LOG}->verbose ("Changes to ", *$self->{filename}, ":");
+        $diff = "" if (! defined($diff));
         *$self->{LOG}->report ($diff);
-
     }
 
     if (*$self->{save}) {

src/main/perl/Log.pm

@@ -6,14 +6,29 @@
 package CAF::Log;
 
 use strict;
-use CAF::Object;
-use vars qw(@ISA);
+use warnings;
+
+use CAF::Reporter qw($SYSLOG);
+use parent qw(CAF::Object Exporter);
+
 use LC::Exception qw (SUCCESS throw_error);
 use FileHandle;
+use Readonly;
+
+Readonly our $FH => 'FH';
+Readonly our $FILENAME => 'FILENAME';
+Readonly my $TSTAMP => 'TSTAMP';
+Readonly my $OPTS => 'OPTS';
+
+our @EXPORT_OK = qw($FILENAME $FH);
+
+# $FH is used during DESTROY (and close), but might be
+# destroyed itself e.g. during global cleanup
+my $_FH = $FH;
 
 my $ec = LC::Exception::Context->new->will_store_all;
 
-@ISA = qw(CAF::Object);
+# TODO: the pod used to say: INHERITANCE: CAF::Reporter
 
 =pod
 
@@ -26,75 +41,70 @@ CAF::Log - Simple class for handling log files
 
   use CAF::Log;
 
-  my $log=CAF::Log->new('/foo/bar','at');
+  my $log = CAF::Log->new('/foo/bar', 'at');
 
   $log->print("this goes to the log file\n");
   $log->close();
 
-=head1 INHERITANCE
-
-  CAF::Reporter
-
 =head1 DESCRIPTION
 
 The B<CAF::Log> class allows to instantiate objects for writing log files.
 A log file line can be prefixed by a time stamp.
 
-
-=over
-
-=cut
-
-#------------------------------------------------------------
-#                      Public Methods/Functions
-#------------------------------------------------------------
-
-=pod
-
-=back
-
 =head2 Public methods
 
 =over 4
 
-=item close(): boolean
+=item C<close()>: boolean
 
-closes the log file.
+closes the log file, returns SUCCESS on success, undef otherwise
+(if no FH attribute exists).
 
 =cut
 
-sub close ($) {
-  my $self=shift;
+# Called during DESTROY, use $_XYZ flavour
+sub close ($)
+{
+    my $self = shift;
 
-  return undef unless (defined $self->{'FH'});
-# Why adding extra newlines???
-#   $self->{'FH'}->print("\n");
-  $self->{'FH'}->close();
-  $self->{'FH'} = undef;
+    return unless (defined $self->{$_FH});
 
-  return SUCCESS;
+    $self->{$_FH}->close();
+    $self->{$_FH} = undef;
+
+    return SUCCESS;
 }
 
+=pod
 
+=item C<print($msg)>: boolean
 
-=pod
+Prints C<$msg> into the log file.
 
-=item print($string):boolean
+If C<TSTAMP> attribute is defined (value is irrelevant),
+a C<YYYY/MM/DD-HH:mm:ss> timestamp and additional space
+are prepended.
 
-prints a line into the log file.
+No newline is added to the message.
+
+Returns the return value of invocation of FH print method.
 
 =cut
 
-sub print ($$) {
-  my ($self,$msg) = @_;
+# TODO: use 'if ($self->{$TSTAMP})' rather than only checking if defined
+
+sub print ($$)
+{
+    my ($self, $msg) = @_;
 
-  if (defined $self->{'TSTAMP'}) {
-    # print timestamp the SUE way ;-)
-    my ($sec, $min, $hour, $mday, $mon, $year) = localtime(time);
-    $msg = sprintf("%04d/%02d/%02d-%02d:%02d:%02d %s",
-		   $year+1900, $mon+1, $mday, $hour, $min, $sec,$msg);
-  }
-  return $self->{'FH'}->print($msg);
+    if (defined $self->{$TSTAMP}) {
+        # print timestamp the SUE way ;-)
+        my ($sec, $min, $hour, $mday, $mon, $year) = localtime(time);
+        $msg = sprintf("%04d/%02d/%02d-%02d:%02d:%02d %s",
+                       $year+1900, $mon+1, $mday, $hour, $min, $sec,$msg);
+    }
+
+    return $self->{$FH}->print($msg);
 }
 
 
@@ -106,82 +116,94 @@ sub print ($$) {
 
 =over 4
 
-=item _initialize($filename,$options)
+=item C<_initialize($filename, $options)>
+
+C<$options> is a string with magic letters
+
+=over
+
+=item a: append to a logfile
 
-initialize the object. Called by new($filename,$options).
+=item w: truncate a loglfile
 
-$options can be 'a' for appending to a logfile, and 'w' for
-truncating, and 't' for generating a timestamp on every
-print. If the 'w' option is used and there was a previous
+=item t: generate a timestamp on every print
+
+=back
+
+Only one of C<w> or C<a> can and has to be set. (There is no default.)
+
+If the C<w> option is used and there was a previous
 log file, it is renamed with the extension '.prev'.
 
 Examples:
-open('/foo/bar','at'): append, enable timestamp
-open('/foo/bar','w') : truncate logfile, no timestamp
+    CAF::Log->new('/foo/bar', 'at'): append, enable timestamp
+    CAF::Log->new('/foo/bar', 'w') : truncate logfile, no timestamp
 
+If the filename ends with C<.log>, the C<SYSLOG> attribute is set to
+basename of the file without suffix (relevant for L<CAF::Reporter::syslog>).
 
 =cut
 
-sub _initialize ($$$) {
-  my ($self,$filename,$options) = @_;
-
-  $self->{'FILENAME'} = $filename;
-  $self->{'OPTS'} = $options;
-
-  if ($self->{FILENAME} =~ m{([^/]*).log$}) {
-    $self->{SYSLOG} = $1;
-  }
-
-  unless ($self->{'OPTS'} =~ /^(w|a)t?$/) {
-    throw_error("Bad options for log ".$self->{'FILENAME'}.
-                      ": ".$self->{'OPTS'});
-    return undef;
-  }
-
-  if ($self->{'OPTS'} =~ /t/) {
-    $self->{'TSTAMP'}=1;
-  }
-
-  if ($self->{'OPTS'} =~ /w/) {
-    #
-    #  Move old filename away if mode is 'w'.
-    #
-    rename ($self->{'FILENAME'},$self->{'FILENAME'}.'.prev')
-      if (-e $self->{'FILENAME'});
-    unless ($self->{'FH'} = FileHandle->new(">".$self->{'FILENAME'})) {
-      throw_error("Open for write ",$self->{'FILENAME'});
-      return undef;
+sub _initialize ($$$)
+{
+    my ($self, $filename, $options) = @_;
+
+    $self->{$FILENAME} = $filename;
+    $self->{$OPTS} = $options;
+
+    if ($self->{$FILENAME} =~ m{([^/]*).log$}) {
+        $self->{$SYSLOG} = $1;
+    }
+
+    unless ($self->{$OPTS} =~ /^(w|a)t?$/) {
+
+        throw_error("Bad options for log ".$self->{$FILENAME}.
+                    ": ".$self->{$OPTS});
+        return;
+    }
+
+    if ($self->{$OPTS} =~ /t/) {
+        $self->{$TSTAMP} = 1;
     }
-  } else {
-    #
-    #  Mode is 'a'. Append to (potentially existing) file
-    #
-    unless ($self->{'FH'} = FileHandle->new(">> ".$self->{'FILENAME'})) {
-      throw_error("Open for append: $self->{'FILENAME'}", $!);
-      return undef;
+
+    my ($fhmode, $msg);
+    if ($self->{$OPTS} =~ /w/) {
+        # Move old filename away if mode is 'w'.
+        rename ($self->{$FILENAME}, $self->{$FILENAME}.'.prev')
+            if (-e $self->{$FILENAME});
+        $fhmode = ">";
+        $msg = "write";
+    } else {
+        # setting is 'a': append to (potentially existing) file
+        $fhmode = ">>";
+        $msg = "append";
     }
-  }
-  #
-  # Autoflush on
-  #
-  $self->{'FH'}->autoflush();
 
-  return SUCCESS;
+    unless ($self->{$FH} = FileHandle->new("$fhmode ".$self->{$FILENAME})) {
+        throw_error("Open for $msg " . $self->{$FILENAME} . " $!");
+        return;
+    }
 
+    # Autoflush on
+    $self->{$FH}->autoflush();
+
+    return SUCCESS;
 }
 
 =pod
 
 =item DESTROY
 
-called during garbage collection. Invokes close()
+Called during garbage collection. Invokes close().
 
 =cut
 
-
+# All Readonly here (and in methods called) might be
+# cleaned up during global cleanup, so use the $_XYZ
+# flavours here (and methods called here). 
 sub DESTROY {
-  my $self = shift;
-  $self->close() if (defined $self->{'FH'});
+    my $self = shift;
+    $self->close() if (defined $self->{$_FH});
 }
 
 =pod
@@ -190,6 +212,8 @@ sub DESTROY {
 
 =cut
 
+# TODO: these are only send to STDERR, not logged
+#       move this to DESTROY?
 
 END {
     # report all stored warnings