these are perhaps style issues (TIMTOWTDI), but they set off red flags for me
whenmy $server_root_dispersion_binary = substr($received_data, 64, 32); my $server_root_dispersion = oct("0b".$server_root_dispersion_binary);
is shorter and at least as readable, and $server_root_dispersion_binary is never actually used again for anything?my $server_root_dispersion = oct("0b" . substr($received_data, 64, 32) +);
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, 32, 32));
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 instancemy $server_precision = oct("0b" . substr($received_data, 24, 8)); my $server_root_delay = oct("0b" . substr($received_data, 24+8, 32));
but there's a bigger problem in that you're repeating this pattern 11 times.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;
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 mapif ($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); ...
never mind that whole sequence of substr calls is actually screaming for you to use unpack, i.e.,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, ... )
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 tomy ($server_li, $server_vn, $server_mode, $server_stratum, ... ) map { oct("0b".$_) } unpack 'a2a3a3a8...', $received_data
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;
In reply to Re: UDP SNTP Client/Server RFC
by wrog
in thread UDP SNTP Client/Server RFC
by thanos1983
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |