On Fri, 9 Apr 2010, Brian Gernhardt wrote:
> Since the same condition is used twice and getting complex, let's move
> it to a new function.
Good idea.
Note that the comments below are just nitpicking about Perl style.
> @@ -863,14 +863,19 @@ sub sanitize_address
> # This maildomain*() code is based on ideas in Perl library Test::Reporter
> # /usr/share/perl5/Test/Reporter/Mail/Util.pm ==> sub _maildomain ()
>
> +sub valid_fqdn
> +{
> + my $domain = $_[0];
> + return !($^O eq 'darwin' && $domain =~ /\.local$/) && $domain =~ /\./;
> +}
A matter of style: in Perl it more usual to use
sub <name> {
...
}
style rather than
sub <name>
{
...
}
Unfortunately git-send-email.perl is a bit inconsistent in the style used;
23 subroutines use Perl style, 5 subroutines including previous one i.e.
sanitize_address use C-like style (one of).
Also, the usual way of unrolling @_; is to use either
my ($par1, $par2, ...) = @_;
or use
mu $par = shift;
The form $_[0] etc. is used very rarely. I think it is even against
Perl Best Practices (see http://www.perlcritic.org and Perl::Critic).
So in my opinion this fragment should be:
+sub valid_fqdn {
+ my $domain = shift;
+ return !($^O eq 'darwin' && $domain =~ /\.local$/) && $domain =~ /\./;
+}
> +
> sub maildomain_net
> {
> my $maildomain;
>
> if (eval { require Net::Domain; 1 }) {
> my $domain = Net::Domain::domainname();
> - $maildomain = $domain
> - unless $^O eq 'darwin' && $domain =~ /\.local$/;
> + $maildomain = $domain if valid_fqdn( $domain );
> }
>
> return $maildomain;
> @@ -887,8 +892,7 @@ sub maildomain_mta
> my $domain = $smtp->domain;
> $smtp->quit;
>
> - $maildomain = $domain
> - unless $^O eq 'darwin' && $domain =~ /\.local$/;
> + $maildomain = $domain if valid_fqdn( $domain );
>
> last if $maildomain;
> }
Style: usually there is no space around function arguments, so
'valid_fqdn($domain);'.
--
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html