Re^2: RFC: Net::SNTP::Client v1

[thanos1983]

Hello Monk::Thomas,

Thank you for your time and effort reviewing my code. Nice point, I forgot that port 0 is also counted.

I have updated all my die evaluations, probably by 99.9% you are right. After the die process it will not return anything. I am using eval to capture the error.

a) Decide to use a specific indentation style and then stick to it. The outer if-conditional has its codeblock indented, the inner one does not. That's very confusing.

You are right, I have updated the documentation also based on Plain Old Documentation format I hope it is correct until I test it. :D

b) Using a 'true' value to indicate an error and a 'false' value to indicate success is counter-intuitive. Using '0' or 'undef' as the error-returns opens up the possibility to use the return value to provide more detail. (e.g. number of returned matches, length of read data, in your case it could be used to indicate the actual port used in case the default applies.)

The reason that I am using TRUE and FALSE is for simplicity and readability reasons in future. I think it makes it easier to understand in case of TRUE do that or in case of FALSE do that.

[Monk::Thomas]

The reason that I am using TRUE and FALSE is for simplicity and readability reasons in future. I think it makes it easier to understand in case of TRUE do that or in case of FALSE do that.

You can keep on doing that. Just switch the meaning of true and false. Have a look at read() http://perldoc.perl.org/functions/read.html

If you're not interested in the actual return code you can simply write

read $fh, 5, 10
  or die;

Or if you want to make sure you read exactly x bytes, then you can write

my $bytes = 5;
my $count = read $fh, $bytes, 10;

if ($count == $bytes) {
  (do stuff)
}
else {
  die sprintf "Want %i bytes, got %s!", $bytes, $count // '<undef>';
}

(The // operator requires at least Perl 5.10.)

[thanos1983]

Hello Monk::Thomas,

Thank you again for your time and effort reviewing my code.

I can not understand why you recommend read

, I am not reading or writing to a file. I am using TRUE or FALSE to determine if the output of the subroutine.

In further analysis, I am using the TRUE and FALSE in order to test for example if the port number is correct, which will result e.g. that the port is ok or maybe is not ok. Then based on the return (TRUE or FALSE) I have an if condition that will return $error if and only if the return from the function was TRUE.

Please correct me if I have miss understood the way you are thinking of using read.

[Monk::Thomas]

read() is just an example for the thing I am recommending. Okay, let's take it from the top. Your code:

    my $verify_port = sub {
    my $port = shift;
    if ($port) {
        if ($port > MAX_PORT or $port < MIN_PORT){
        return TRUE;
        }
    }
    else {
        $moduleInput{-port} = 123;
        return FALSE;
    }
    };

If the port is outside the valid range, then you return true, if it is inside the allowed port range then you return (I am not sure - if no explicit return value is specified then the one from the last assignment is chosen - so - $port?) and if the port is undefined you set some value and return false.

btw. the '$moduleInput{-port} = 123;' thing is bad style because you're modifying some completely unrelated piece of data. It's a lot better to make it obvious when some value is changed.

This is how I would tackle this:

# I ~love~ readonly. Primarily because you can write stuff like:
# print "$MIN_TCP_PORT < x < $MAX_TCP_PORT\n";
use Readonly;

Readonly my $DEFAULT_NTP_PORT => 123;
Readonly my $MIN_TCP_PORT     => 1;
Readonly my $MAX_TCP_PORT     => 65535;

# in case of success: return a 'true' value   (1 or a useful value)
# in case of error:   return a 'false' value  (0 or undef)
sub verify_port {
  my $value = shift;

  # user provided input should be viewed as toxic - do not
  # trust until proven to contain a sane value
  if (defined $value and $value =~ /\A (\d+) \z/xms) {
    my $port = $1;

    if ($MIN_TCP_PORT <= $port and $port <= $MAX_TCP_PORT) {
      return 1;  # or TRUE or $port
    }
  }

  return 0;  # or FALSE or undef
}

# get a port value from somewhere, e.g. Getopt::Long
# use a default value if no value has been provided
my $input = $opt_ntp_port // $DEFAULT_NTP_PORT;

# - usage a -
if (verify_port($input)) {
  (do stuff with $input)
}
else {
  (issue warning / bail out)
}

# - usage b -
verify_port($input)
  or die;

# - usage c -
# (possible if verify_port() returns $port)
my $ntp_port = verify_port($input)
  or die 'Invalid ntp port!';

This is a similar behaviour to read() and that's why I mentioned it.

Usage c would not work if the returned value can be 0. Luckily 0 is not an allowed port for TCP!