more comments about reputer

OK, now I understand why things are formatted as they are. I suppose this also explains why you don't indent subroutines :-) What I do know is I edit in terminal windows that I stretch out to 50 rows by 110 columns or more, so even with loads of whitespace I can see a lot of information at once.

Some more comments, now that I've looked at the code in more detail.

Update: more comments added 23-jul-2001

This first point is pretty anal-retentive, so don't take it too seriously. This gets back to the whitespace argument, but ceteris parabus I don't believe you lose anything. The point is I find...

  my$temp = '/home/dlandgre/perl/reputer/'; # where data files are sav
+ed
  my$public_access = ''; # 'yes' disables config, downloads, and exter
+nal program functions
  my$bodytag = '<body bgcolor="#aaaaaa" text="#000000">';
  my$form_method = 'get'; # get or post, 'get' shows params in url
[download]

...much harder to scan than...

  my$temp          = '/home/dlandgre/perl/reputer/'; # where data file
+s are saved
  my$public_access = ''; # 'yes' disables config, downloads, and exter
+nal program functions
  my$bodytag       = '<body bgcolor="#aaaaaa" text="#000000">';
  my$form_method   = 'get'; # get or post, 'get' shows params in url
[download]

In a similar vein, there are a number of areas in the code where you initialise a whole slew of variables at once....
```
my($mode,$td,$ta,$rd,$ra,$ca,$cd) = '';
[download]
```
...although this only initialises the first variable in the list. (I think this was brought up in the CB, but I didn't pay close attention). Anyway, my first cut at getting it right was...
```
my($mode,$td,$ta,$rd,$ra,$ca,$cd) = ('','','','','','','');
[download]
```
I ended up, however, doing something else (when I wound up being off by one). Where you may agree to disagree with me (given that it chews up a lot more space, I much prefer reading...
```
my $mode = '';
my $td   = '';
my $ta   = '';
[download]
```
... it just stands out more clearly.
Using unless to introduce large conditional blocks. I don't really like that. Rather than...
```
  unless($i{'n'} eq 'logout'){
    if($ins > 1){$s='s'}
    &begin_html('fu');
    print qq~$bq $bq $bq
[download]
```
...I prefer to reverse the comparison operator and say if ($i{'n'} ne 'logout'). I tend to use unless only in the following scenario...
```
  $var += 2 unless $condition;
[download]
```
... i.e. trivial conditions.
Similarly if($ins > 1){$s='s'} is better written as $s='s' if $ins > 1 and as an added benefit you avoid the cost of building up and tearing down a code block.

You are using Data::Dumper to serialise the data between invocations. Are you aware of Storable? It's much faster. I wrote a benchmark to load the reputer.dat file, increment the reputation of a node and then store it out again. I had to hack the data structure a bit, because Storable wants to serialise one reference only per file.

#! /usr/bin/perl -w

use strict;
use Data::Dumper;

$Data::Dumper::Indent = 0;
use vars qw/$xpdat1 $dat1 $makedat/;

  my $datafile = shift or
    die "No data file specified on command line.\n";
  my $today = localtime(time);

  my $root;
  eval "require '$datafile'" or die "bad eval: $@\n";

  $root->{DATA} = $dat1;
  $root->{XP}   = $xpdat1;
  $root->{MAKE} = $makedat;

  open DAT, ">$datafile" or die "Cannot open $datafile for output: $!\
+n";

  print DAT Data::Dumper->Dump( [$root], ['$root'] );
  close DAT;
[download]

Note that the...

  print DAT Data::Dumper->Dump( [$root], ['$root'] );
[download]

...construct saves out the data structure with the correct name, as opposed to using...

  $var = 'root';
  $Data::Dumper::Varname = "$var";
  print DAT Dumper($root);
[download]

...with the funny $xpdat1 $xpdata dot-and-carry-one method. Once the data is encoded in this manner, we can then begin to compare Data::Dumper versus Storable.

  #! /usr/bin/perl -w

  use strict;

  use Data::Dumper;
  use Storable;
  use Benchmark;

  use vars qw/$root $root1/;

  my $ddi = 0; # data dumper intent level, 0 = smallest files
  my $var;

  my $datafile = shift or die "No data file specified on command line.
+\n";
  my $today = localtime(time);

  my $storefile = $datafile . '.store';

  eval "require '$datafile'" or die "bad eval: $@\n";
  $root = $root1;
  store $root, $storefile;

  my $debug = 0;

  sub via_eval_dump {
    eval "require '$datafile'" or die "bad eval: $@\n";
    open DAT, ">$datafile" or die "Cannot open $datafile for output: $
+!\n";
    $root = $root1;

    $root->{DATA}{NODE}[45]{'reputation'} += 1;
    $debug and print "$root->{DATA}{NODE}[45]{'content'} $root->{DATA}
+{NODE}[45]{'reputation'} \n";

    $Data::Dumper::Indent = $ddi;
    $var = 'root';
    $Data::Dumper::Varname = "$var";
    print DAT Dumper($root);
    close DAT;
  }    

  sub via_retrieve_store {
    $root = retrieve $storefile;
    $root->{DATA}{NODE}[45]{'reputation'} += 1;
    $debug and print "$root->{DATA}{NODE}[45]{'content'} $root->{DATA}
+{NODE}[45]{'reputation'} \n";
    store $root, $storefile;
  }

  timethese shift(), {
    'eval & dump'      => \&via_eval_dump,
    'retrieve & store' => \&via_retrieve_store,
  }
[download]

Bear in mind though, that the datafile produced by Storable is not human-readable. You have to write companion scripts to repackage it via Data::Dumper if you need to eyeball it afterwards. The results are very eloquent...

% ./benchstore reputer.dat 5000
Benchmark: timing 5000 iterations of eval & dump, retrieve & store...
eval & dump: 536 wallclock secs (533.99 usr +  1.51 sys = 535.50 CPU)
retrieve & store: 29 wallclock secs (26.67 usr +  2.71 sys = 29.38 CPU)

Once again, are you interested in diff patches? Should I post them to the reputer code node?

Hmm, I guess that's enough rambling for one day. This is a darn useful piece of code and I'm surprised more people aren't playing with it (and making suggestions).

update 23-jul-2001

I rewrote the graph routine. All I really wanted to do was to put a spacer between the highlighted number and the histogram in the next column so that they don't run on into each other. I wound up coalescing other bits and pieces to make things fit more cleanly in the code.

sub graph
{ # calculate and display the graphs
my($hm1,$hm2,$hm3,$bdr,$cs,$spacer,$mode,$descr);

if(!$i{histmode} or $i{histmode} == 1){ 
    $hm1 = ' checked';
    $spacer = '<td width="1"></td>';
    $cs = 4;
    $bdr = 0;
    $descr = qq~<b>mode one</b><br><small>
    bar height = fixed<br>
    bar width = number of nodes at that rep<br></small></td></tr>~
    }
elsif($i{histmode} == 2){ 
    $hm2 = ' checked';
    $spacer = '';
    $cs = 3;
    $bdr = 1;
    $descr = qq~<b>mode two</b><br><small>
    bar height = rep<br>
    bar width = number of nodes at that rep<br></small></td></tr>~
    }
elsif($i{histmode} == 3){
    $hm3 = ' checked';
    $spacer = '<td width="1"></td>';
    $cs = 4;
    $bdr = 0;
    $descr = qq~<b>mode three</b><br><small>
    bar height = number of nodes at that rep<br>
    bar width = rep<br></small></td></tr>~
    }

print qq~<p><table border="$bdr" cellpadding="0" cellspacing="0" width
+="100%">
<tr><th align="left" colspan="$cs" valign="top">
<table border="0" cellpadding="0" cellspacing="0" width="100%" bgcolor
+="#cfcfcf">
<tr><th align="left">&nbsp;&nbsp;
<h2>Number of nodes by reputation
<font size="-1"><br>average rep and maximum num highlighted</font></h2
+></th>
<td><table align="right" border="0" cellpadding="3" cellspacing="0"><t
+r><td>$descr
<tr><form method="$form_method"><td align="right" valign="bottom">
1<input type="radio" name="histmode" value="1"$hm1> 
2<input type="radio" name="histmode" value="2"$hm2> 
3<input type="radio" name="histmode" value="3"$hm3> 
<input type="hidden" name="n" value="graph">
<input type="submit" value="mode"></td></form></tr></table>
</td></tr></table></td></tr>~;

my@high = sort {$b <=> $a} values %rep_freq;
my@hig  = sort {$b <=> $a} keys %rep_freq;
my$mult = sprintf "%d", (600/$high[0]); # normalize bar width to scale
+ (highest num = 600 pixels)
my$mul  = sprintf "%d", (600/$hig[0]); # normalize bar width to scale 
+(highest num = 600 pixels)
print qq~<tr bgcolor="#b0b0b0"><td><b>$nb rep $nb</td><td><b>$nb num $
+nb</td><td>&nbsp;</td></tr>~;

my($ar,$fc,$arf,$fcf) = '';

for(sort {$b <=> $a} keys %rep_freq){ 
    my$w = ($rep_freq{$_}*$mult);
    my$h = 5;
    if($i{'histmode'}){
        if($i{'histmode'} == 2){ $h = $_ || 1 }
        elsif($i{'histmode'} == 3){ $w = ($_*$mul); $h = $rep_freq{$_}
+ }
        }
    if($_ == $avgrep){ # highlight avg rep
        $ar = 'bgcolor="#880066"'; 
        $fc = '<font color="white">'
        } 
    else{$ar=''; $fc='';}
    
    if($rep_freq{$_} == $high[0]){ # highlight frequency high
        $arf = 'bgcolor="#880066"'; 
        $fcf = '<font color="white">'
        }
    else{$arf=''; $fcf=''}

    print qq~<tr><td $ar align="right">$fc $_ $nb</td><td $arf align="
+right">$fcf $rep_freq{$_} $nb</td>
$spacer<td><table border="0" cellpadding="0" cellspacing="0">
<tr><td bgcolor="#880066"><img src="$uri?n=gif" width="$w" height="$h"
+ border="0"></td></tr></table>
</td></tr>~
    }
print qq~<tr bgcolor="#b0b0b0">
<td><b>$nb rep $nb</td><td><b>$nb num $nb</td><td>&nbsp;</td></tr></ta
+ble><p>~;
}
[download]

--
g r i n d e r

Comment on more comments about reputer Select or Download Code


Problems? Is your data what you think it is?
	PerlMonks