| Subject: | [PATCH] Convert away from some trailing if/whiles to make the code less clever and easier to read. |
Hi,
reading the Parser-MGC code gave me quite a few "WTF?"/"What does it
mean?" moments due to the many trailing ifs combined with "or"s or
"ands", which are hard to parse. This patch converts some of them to a
saner if (COND()) { BLOCK } format.
I have a policy against most trailing ifs/unlesses/whiles/untils in my
own code and avoid putting too much into a single statement. It is a
good idea to optimise for readability instead of cleverness.
Regards,
-- Shlomi Fish
| Subject: | Parser-MGC-less-WTF-moments.patch |
diff -r b88427c6fdcf lib/Parser/MGC.pm
--- a/lib/Parser/MGC.pm Sun Dec 09 16:11:47 2012 +0200
+++ b/lib/Parser/MGC.pm Sun Dec 09 16:30:02 2012 +0200
@@ -288,8 +288,12 @@
my $str = $self->{str};
my $sol = $pos;
- $sol-- if $sol > 0 and substr( $str, $sol, 1 ) =~ m/^[\r\n]$/;
- $sol-- while $sol > 0 and substr( $str, $sol-1, 1 ) !~ m/^[\r\n]$/;
+ if ($sol > 0 and substr( $str, $sol, 1 ) =~ m/^[\r\n]$/) {
+ $sol--;
+ }
+ while ($sol > 0 and substr( $str, $sol-1, 1 ) !~ m/^[\r\n]$/) {
+ $sol--;
+ }
my $eol = $pos;
$eol++ while $eol < length($str) and substr( $str, $eol, 1 ) !~ m/^[\r\n]$/;
@@ -417,8 +421,10 @@
my $e = $@;
pos($self->{str}) = $pos;
-
- die $e if $committed or not eval { $e->isa( "Parser::MGC::Failure" ) };
+ if ($committed or
+ (! eval { $e->isa( "Parser::MGC::Failure" ) })) {
+ die $e;
+ }
return undef;
}
@@ -497,7 +503,10 @@
my $self = shift;
my ( $sep, $code ) = @_;
- ref $sep or $sep = qr/\Q$sep/ if defined $sep;
+ # Convert $sep into a regular expression in case it is a string.
+ if (defined $sep and !ref $sep) {
+ $sep = qr/\Q$sep/;
+ }
my $committed;
local $self->{committer} = sub { $committed++ };
@@ -512,7 +521,9 @@
my $e = $@;
pos($self->{str}) = $pos;
- die $e if $committed or not eval { $e->isa( "Parser::MGC::Failure" ) };
+ if ($committed or not eval { $e->isa( "Parser::MGC::Failure" ) }) {
+ die $e;
+ }
last;
}
continue {
@@ -588,12 +599,16 @@
local $self->{committer} = sub { $committed++ };
my $ret;
- eval { $ret = shift->( $self ); 1 } and return $ret;
+ if (eval { $ret = shift->( $self ); 1 }) {
+ return $ret;
+ }
my $e = $@;
pos( $self->{str} ) = $pos;
- die $e if $committed or not eval { $e->isa( "Parser::MGC::Failure" ) };
+ if ($committed or not eval { $e->isa( "Parser::MGC::Failure" ) }) {
+ die $e;
+ }
}
$self->fail( "Found nothing parseable" );