Re: [PATCH 2/2] Git/Packet.pm: use 'if' instead of 'unless'

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 21, 2017, 19:24 UTC
Message-ID: <20171121192440.GE3429@aiede.mtv.corp.google.com>
In-Reply-To: <20171121160939.22962-2-chriscool@tuxfamily.org>

Hi,

Christian Couder wrote:

Show 6 quoted lines

> The code is more understandable with 'if' instead of 'unless'.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  perl/Git/Packet.pm | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

I'm agnostic about that. In some ways it seems like an odd change, since it is changing from

	if (exceptional case) {
		die "explanation";
	}
	common case;

to

	if (!exceptional case) {
		common case;
	}
	die "explanation";

when we usually prefer the former (getting the exceptional case over with early so the reader can concentrate on the common case). In perl, it might be even more idiomatic to do

	die "explanation" if exceptional case;
	common case;

but Documentation/CodingGuidelines appears to recommend not going that far. :)

[...]

Show 7 quoted lines

> --- a/perl/Git/Packet.pm
> +++ b/perl/Git/Packet.pm
> @@ -68,16 +68,16 @@ sub packet_bin_read {
>  
>  sub remove_final_lf_or_die {
>  	my $buf = shift;
> -	unless ( $buf =~ s/\n$// ) {

For readability, I find this whitespace within parens more distracting.

Documentation/CodingGuidelines covers that for C programs but not for Perl. Do we have a preferred style either way about it?

Thanks, Jonathan