Hi,
Cryptkiddy wrote (17 Jul 2011 21:25:06 GMT) :
> the git-diff with the new patch is attached. I think I managed to
> fix everything you told me to.
Thanks. Sorry for the delay. Since you're asking, I'll go on playing
the Perl teacher bellow, instead of fixing and merging stuff myself ;)
> I am still not sure about the errorhandling part (Do you think it is
> OK to inline that the way I did?) .
Well, it's better.
> + $router_port = $i2p_default_port;
> + return $router_port;
could be shortened to
return $router_port = $i2p_default_port;
... but all this repeated pattern is actually a symptom of your
piggy-back of the state variable feature to store the function return
value, once it's been computed.
Had you used Memoize (https://metacpan.org/module/Memoize), which I
still recommend since it's exactly the right tool for the task at
hand, you could just:
return $i2p_default_port;
... which expresses much more clearly your intentions I think.
> If there is anything else I can do to make it better just let me
> know.
See my comments bellow.
> + my $client_config = join("", <$conffile>);
> + close($conffile);
> +
> + my $client_app_no;
> + my $regex = '\nclientApp\.([0-9]+)\.main=net\.i2p\.router\.web\.RouterConsoleRunner\n';
It is very non-idiomatic to first read the whole file into a
multi-line string, then apply a regex on it to extract a sub-string.
A more idiomatic way to do it is:
my $regex = '^clientApp\.([0-9]+)\.main=net\.i2p\.router\.web\.RouterConsoleRunner$';
while ( my $line = <$conffile> ) {
chomp $line;
if (($client_app_no) = ($line =~ m/$regex/)) {
}
Also, you're binding this $regex once per line until it matches or the
end of the file (whatever happens first), so it's good practice to
compile it once for all. See qr// in perlop(1).
> + $regex = '\nclientApp\.' . $client_app_no . '\.args=([0-9]{1,5})\ [0-9s:,. \-]+ \.\/webapps\/[\s]*\n';
Regarding [0-9s:,. \-], I still doubt you really want to include the
"\" character in the caracter class.
Also, preferably use \s or [[:space:]], or even [[:space:]]+, instead
of "\ ". It's both clearer to read, and likely to be more robust on
the long run especially when when parsing files you don't control.
The "+" I suggest also allows you to remove the space in the character
class that follows.
Regarding "[\s]*", if you mean "possibly followed by space(s)", I
think "[[:space:]]*" is clearer as it does not require the reader to
remember if \s is interpreted or not inside a character class.
Bye,
--
intrigeri <intrigeri@???>
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
| We're dreaming of something else.
| Something more clandestine, something happier.
