Re: [Tails-dev] ***SPAM*** Re: Proposed changes to tails-sta…

Delete this message

Reply to this message
Autor: intrigeri
Data:  
A: Cryptkiddy
CC: tails-dev
Assumpte: Re: [Tails-dev] ***SPAM*** Re: Proposed changes to tails-starter-i2p && my first contribution to OSS
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.