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

  1. 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);
    when
    my $server_root_dispersion = oct("0b" . substr($received_data, 64, 32) +);
    is shorter and at least as readable, and $server_root_dispersion_binary is never actually used again for anything?

  2. 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));
    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));
    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;
    but there's a bigger problem in that you're repeating this pattern 11 times.

  3. 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); ...
    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, ... )
    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
    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

  4. 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;
And then you might want to have the %srv hash be an object...

In reply to Re: UDP SNTP Client/Server RFC by wrog
in thread UDP SNTP Client/Server RFC by thanos1983

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":



  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.