in reply to RFC: Net::SNTP::Server v1

I have so far only read most of the code, not the documentation. Here are a couple of issues I noticed with the code. (Code snippets are untested.)

  1. BEGIN { die 'Perl version 5.6.0 or greater is required' if ($] < 5.006); } can be replaced by use 5.006;. Also, unless perlver is mistaken, the minimum required version for your code is v5.8.0.
  2. use base qw( Exporter ); our @ISA = qw ( Exporter );: You don't need to use base and modify @ISA. Also, that code can be replaced by simply use Exporter "import";; or, if you really want to inherit from Exporter, use parent instead of base.
  3. $dot is used only once, and is so simple that it should be inlined.
  4. Code like local *checkHashKeys = sub {: Don't do that without a good reason (and I don't see one). Use a "private" sub instead (a regular sub whose name begins with an underscore and that isn't exported). Several of your subs of this form are used only once and can and probably should be inlined.
  5. Your dec_2_hex can be replaced by sprintf('%02X%02X%02X%02X',$a,$b,$c,$d) or maybe join('',map {sprintf '%02X', $_} $a,$b,$c,$d) (note the uppercase X).
  6. Code like return ($error = "Not defined IP", \%moduleInput): $error will go out of scope immediately after the return, so assigning to it is not useful and confusing to readers, since it's not immediately apparent whether $error might be a package variable or not (which would actually make more sense than it being a lexical, as it currently is).
  7. Error handling in general: While the return values of your sub are certainly your choice, what you've chosen is uncommon. Perhaps you should be throwing errors with croak (Carp; that's how I'd likely do it), or, if you don't want to throw errors, return nothing (return;) and set a special error variable.
  8. Lots of unused variables, and several variables used only once like @arraySendSntpPacket that can be inlined.
  9. The body of your main while loop is not indented properly and you have several very long lines that should probably be wrapped.

Will you be releasing tests with this module?

Replies are listed 'Best First'.
Re^2: RFC: Net::SNTP::Server v1
by thanos1983 (Parson) on Jul 18, 2015 at 23:14 UTC

    Hello Anonymous,

    Thank you for reviewing my code.

    Well I am using (5.006) because in the source code of Time::HiRes, the author using this version. Since I am using the module I assumed I should not have higher version. The die syntax I picked it up from the Net::SNMP source code. I thought it is more tricky and nice to write it like this.

    The only reason that I am using named subroutines, is to assist my self why I did something in future. I know that most of my functions only apply once in the code, but I assumed that this is the way to remember what I was trying to do in future.

    Unused Variables such as @arraySendSntpPacket etc. could be inlined, but they help me to debug the code easier than defining all the variables one by one and printing them. Also for future when I will be updating the code it would make it easier to remember why e.g. a variable is 0 or 3 because it is li _vn_mode.

    Yes I have prepared a few tests that cam to my mine with the basic version that I want to release. I am planning to release also a bit more complicated version updating the internal clock based on user preference. One step at a time.

    Seeking for Perl wisdom...on the process of learning...not there...yet!
[reply]
[d/l]
[select]
      Since I am using the module I assumed I should not have higher version

      I can see how one might arrive that conclusion, but it's incorrect.
      "use 5.006;" is there to ensure that 5.006 or higher is in use - that's it's only intent.
      IMO it's an unnecessary stipulation, and it certainly doesn't mean that you should use perl 5.6.

      Cheers,
      Rob
[reply]

In Section Meditations