Beefy Boxes and Bandwidth Generously Provided by pair Networks
There's more than one way to do things
 
PerlMonks  

more comments about reputer

by grinder (Bishop)
on May 22, 2001 at 17:34 UTC ( #82216=note: print w/ replies, xml ) Need Help??


in reply to reputer reply

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

    ...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
  • 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) = '';

    ...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) = ('','','','','','','');

    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 = '';

    ... 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

    ...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;

    ... 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;

    Note that the...

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

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

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

    ...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, }

    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>~; }

--
g r i n d e r


Comment on more comments about reputer
Select or Download Code

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://82216]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others pondering the Monastery: (14)
As of 2014-08-01 14:47 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    Who would be the most fun to work for?















    Results (25 votes), past polls