Re: large expression formating

You have false laziness and not enough hubris. (Which means you are copy-pasting where you should be abstracting.)

The fact you are calling your subs test1, test2, test3 etc should immediately set off the alarms.

As a minor note, whenever you want to return "false" from a function in Perl, you should do that with a blank return. You should neither return 0 nor return "" - because these return a one-element list, which while false in scalar context is true in list context. A blank return will return undef in scalar and an empty list in list context, which is what you want.

my @displacement = (
    [-1, -1],
    [-1,  0],
    [-1, +1],
    [ 0, -1],
    [ 0,  0],
    [ 0, +1],
    [+1, -1],
    [+1,  0],
    [+1, +1],
    [-1, -1],
);

sub check_coord {
    my ($x, $y, $wall, $coord)= @_;

    for($displacement[$coord - 1]) {
        $x += $_->[0];
        $y += $_->[1];
    }

    return
        if $y < 0 or $x < 0
        or not defined(
           $map[$y]
           or $map[$y][$x]
        );

    return $map[$y][$x] eq $wall;
}
[download]

and then you can use grep - this: if ((!&test2($xCur, $yCur, $wall) && !&test3($xCur, $yCur, $wall) && &test5($xCur, $yCur, $wall) && &test6($xCur, $yCur, $wall) && !&test9($xCur, $yCur, $wall))&& (!&test1($xCur, $yCur, $wall) && !&test7($xCur, $yCur, $wall) ) || (&test1($xCur, $yCur, $wall) && &test4($xCur, $yCur, $wall) && &test7($xCur, $yCur, $wall) && !&test8($xCur, $yCur, $wall))) becomes if(grep { check_coord($xCur, $yCur, $wall, $_) } 2..8) {

~~since~~ grep in scalar context returns the number of matches.

You can apply the same translation to the rest of your code.

I recommend you read Mark-Jason Dominus' excellent Program Repair Shop and Red Flags article series on Perl.com.

Makeshifts last the longest.

Comment on Re: large expression formating Select or Download Code

Replies are listed 'Best First'.
Re: Re: large expression formating by Dr.Altaica (Scribe) on Oct 20, 2002 at 22:42 UTC
Thanks about return. That means I need replace: `return $map[$yCur-1][$xCur-1] eq $wall;` with `return if!($map[$yCur-1][$xCur-1] eq $wall);`? but when are you realy going to have a truth in list context? hmm I wounder is perl6 with have a list function.... I don't know much about grep but that apears to be equivalent to `if( &test2($xCur, $yCur, $wall) && &test3($xCur, $yCur, $wall) && &test4($xCur, $yCur, $wall) && &test5($xCur, $yCur, $wall) && &test6($xCur, $yCur, $wall) && &test7($xCur, $yCur, $wall) && &test8($xCur, $yCur, $wall))` [download] It gets me this ((((..( (..(..( (..(..( ((((..( ......( ((((((( (..(..( (..(..( ((((((( (..(..( ((((((( Not this. (.).... ....... ....... (..)... ....... (.)(.). ....... ....... .().(.) ....... ....... and I don't see you doing anything to prevent wraping around the array? \|\_/\| /o o\ (>_<) `-'	[reply] [d/l] [select]
Re^3: large expression formating by Aristotle (Chancellor) on Oct 20, 2002 at 23:41 UTC
No no. `return $map[$yCur-1][$xCur-1] eq $wall;` is fine. But, ouch. You're right about my simplification with grep, I looked far too briefly. Now let's see.. I always get confused by what conditions to use for grep in boolean context, so I'll think aloud. `!$cond1 and !$cond2` means that neither of them must be true. So the number of matches when testing for truth must be zero. That's `not grep { $_ } $cond1, $cond2`. So I was using the wrong one to begin with. And I also overlooked that you weren't simply testing all coordinates for falseness - unraveled, it looks like this: `if ( ( !&test2($xCur, $yCur, $wall) && !&test3($xCur, $yCur, $wall) && &test5($xCur, $yCur, $wall) && &test6($xCur, $yCur, $wall) && !&test9($xCur, $yCur, $wall) ) && ( !&test1($xCur, $yCur, $wall) && !&test7($xCur, $yCur, $wall) ) \|\| ( &test1($xCur, $yCur, $wall) && &test4($xCur, $yCur, $wall) && &test7($xCur, $yCur, $wall) && !&test8($xCur, $yCur, $wall) ) )` [download] That looks decidedly more complex than a simple grep. But since the inner parens only contain &&s, the first two can be combined: `if ( ( !&test2($xCur, $yCur, $wall) && !&test3($xCur, $yCur, $wall) && &test5($xCur, $yCur, $wall) && &test6($xCur, $yCur, $wall) && !&test9($xCur, $yCur, $wall) && !&test1($xCur, $yCur, $wall) && !&test7($xCur, $yCur, $wall) ) \|\| ( &test1($xCur, $yCur, $wall) && &test4($xCur, $yCur, $wall) && &test7($xCur, $yCur, $wall) && !&test8($xCur, $yCur, $wall) ) )` [download] Now that looks almost like a datastructure, so let's make it one. `my %test1 = ( 2 => 0, 3 => 0, 5 => 1, 6 => 1, 9 => 0, 1 => 0, 7 => 0, ); my %test2 = ( 1 => 0, 4 => 0, 7 => 1, 8 => 0, );` [download] Now we have good reason to modify the behaviour of the `check_coordinates` function: it isn't returning true/false, it's returning a comparison value. So have it `return 0 if $lots_of_checks;` and `return 1 if $map[$y][$x] eq $wall;`. Then we can rewrite that `if` as: `if( not(grep { check_coord($xCur, $yCur, $wall, $_) ne $test1{$_} } ke +ys %test1) or not(grep { check_coord($xCur, $yCur, $wall, $_) ne $test2{$_} } + keys %test2) ) { }` [download] That looks bulky, but remember it's as bulky as it's going to get. If you need to add more conditions, you add them in the hashes, not the `if`. And rather than `%test1` and `%test2` you should use meaningful names - maybe `%corner_pixels` f.ex or some such, so your variables express your intent. Makeshifts last the longest.	[reply] [d/l] [select]