Re: UDP SNTP Client/Server RFC

these are perhaps style issues (TIMTOWTDI), but they set off red flags for me

You do not have to name every value. In particular, why are you doing

my $server_root_dispersion_binary =  substr($received_data, 64, 32);
my $server_root_dispersion = oct("0b".$server_root_dispersion_binary);
[download]

when

my $server_root_dispersion = oct("0b" . substr($received_data, 64, 32)
+);
[download]

is shorter and at least as readable, and $server_root_dispersion_binary is never actually used again for anything?

Doing your own arithmetic is bad. You have a whole sequence of these where each one picks up where the previous one left off,
```
my $server_precision = oct("0b" . substr($received_data, 24, 8));
my $server_root_delay = oct("0b" . substr($received_data, 32, 32));
[download]
```
where, that first 32 is really 24+8. If you make a mistake/typo and, say, write 33 instead of 32, that would be a bug that you'd then have to find. The computer is much more reliable at doing addition than you'll ever be.
```
my $server_precision = oct("0b" . substr($received_data, 24, 8));
my $server_root_delay = oct("0b" . substr($received_data, 24+8, 32));
[download]
```
which, I'll admit, is not where you want to leave things because now you're repeating the 8 and the 24, which could also get screwed up; now we're in a place where you actually do want to be using a variable to keep track of where you are. Which we could solve in this one instance
```
my $server_precision = oct("0b" . substr($received_data, $here, ($widt
+h = 8)));
$here += $width;
my $server_root_delay = oct("0b" . substr($received_data, $here, ($wid
+th = 32)));
$here += $width;
[download]
```
but there's a bigger problem in that you're repeating this pattern 11 times.
Don't Repeat Yourself. Any time you're programming using copy and paste, you're defeating the whole purpose of writing a program. Let the computer deal with the repetition. Write a loop or capture the common stuff in a subroutine or both, e.g.,
```
if ($received_data) {
   my $here = 0;
   my $next_value = sub { 
      my $width = shift; 
      my $start = $here;
      $here += $width;
      return oct("0b".substr($received_data, $start, $width));
   };
   my $server_li = $next_value->(2);
   my $server_vn = $next_value->(3);
   my $server_mode = $next_value->(3);
   my $server_stratum = $next_value->(8);
   ...
[download]
```
which is still more verbose than I'd like because of all of the calls to $next_value->(), but at least you're not computing any offsets or start positions yourself anymore. You could tighten this up a whole lot more by using map
```
if ($received_data) {
   my $here = 0;
   my ($server_li, $server_vn, $server_mode, $server_stratum, ... ) =
     map { 
        my $start = $here;
        $here += $_;
        return oct("0b".substr($received_data, $start, $_));
     }
      (2, 3, 3, 8, ... )
[download]
```
never mind that whole sequence of substr calls is actually screaming for you to use unpack, i.e.,
```
my ($server_li, $server_vn, $server_mode, $server_stratum, ... )
     map { oct("0b".$_) } unpack 'a2a3a3a8...', $received_data
[download]
```
which then points out another problem here being that the various fields are divorced from their widths, i.e., they're effectively in two separate lists, which could easily get out of sync and make life miserable if one of them ever changes or turns out to be wrong. Which then leads to

Hashes are your friends; use them. But this is an easy fix:


our (@fields, %width, $packt);
{
  my @f = (
    # list of field names and widths in the order in which they appear
    li   => 2, 
    vn   => 3,
    mode => 3,
    (map {$_ => 8}  qw(stratum poll_interval precision)),
    (map {$_ => 32} qw(root_delay dispersion identifier)), 
    (map {$_ => 64} qw(ref_epoch originate_epoch receive_epoch transmi
+t_epoch))
  );

  @fields = map {$f[$_*2]} (0 .. @f/2);
  %width = (@f);
  $packt = join '', map {"a$width{$_}"} @fields;
}

...

%srv = ();
@srv{@fields} = map { oct("0b".$_) } unpack $packt, $received_data;
[download]

And then you might want to have the %srv hash be an object...

Comment on Re: UDP SNTP Client/Server RFC Select or Download Code


XP is just a number
	PerlMonks