in reply to Refactoring challenge.

The first thing is that I'm going to be changing the callers because pp() references global data (namely $str). pp() also alters that data, so I will be returning the altered $str.

The code, upon first glance, looks slightly unfactored. But, I've taken the common element (the actual print command) and moved it out of the sub. This allows for pp() to display the actual parts that differ. For example, you have four separate paths through pp(), depending on how f1() and f2() behave. This version makes that explicit. 

{
    my ($indent, $depth) = ( "\t", 0 );

    my $_print = sub {
        print $indent x $depth, substr( $_[0], 0, $_[1] );
    };

    sub pp {
        return unless @_;
        my ($str) = @_;

        my ($pos, $first ) = f1( $str );
        if ( defined $first ) {
            if ( $first eq CONST1 ) {
                $depth++;
                $_print->( $str, ++$pos );
            }
            elsif ( defined( my $newpos = f2( $str, $first ) ) ) {
                $_print->( $str, $newpos + 2 );
            }
            else {
                $_print->( $str, $pos );
                $depth++;
            }
        }
        else {
            $_print->( $str, $pos );
        }

        $str =~ s[^.{$pos1}\s*][];
        return $str;
    }
}

[download]

Update: Upon further thought, the calls to f1() and f2() need to be refactored further. 

{
    my ($indent, $depth) = ( "\t", 0 );

    my $_print = sub {
        print $indent x $depth, substr( $_[0], 0, $_[1] );
    };

    my $_call_f1_f2 = sub {
        my $str = shift;

        my ($pos, $first) = f1( $str );
        return ($pos, 0) if !defined $first;

        if ( $first eq CONST1 ) {
            $depth--;
            return ($pos + 1, 0);
        }

        $pos = f2( $str, $first );
        return ($newpos + 2, 0) if defined $pos;

        return ($pos, 1);
    };

    sub pp {
        return unless @_;
        my ($str) = @_;

        my ($pos, $depth_mod) = $_call_f1_f2->( $str );

        $_print->( $str, $pos );

        $depth += $depth_mod;

        $str =~ s[^.{$pos1}\s*][];
        return $str;
    }
}

[download]

Update: Typo in code ($_->print -> $_print->) fixed.

Being right, does not endow the right to be rude; politeness costs nothing.
Being unknowing, is not the same as being stupid.
Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence.
Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.

Replies are listed 'Best First'.
Re^2: Refactoring challenge.
by BrowserUk (Patriarch) on Mar 06, 2005 at 02:21 UTC

    Thankyou. That is (almost) perfect.

    Which impresses the heck outta me cos I've rather spent altogether too long staring at that code trying to refactor it and every time I changed it, it broke . Badly.

    And I have had another (very competent) set of eyes look it over (complete with the full context) prior to posting.

    Plugged your version back into the real conetxt and made two changes:

  • The first $depth++ should be $depth--.
  • And $newpos + 2 had to be $pos += $newpos + 2.

    Both trivial typos given the "paper exercise" nature of the task.

    You will see the code in it's proper context over the next couple of weeks or so, but for now. Thankyou.

    Examine what is said, not who speaks.
    Silence betokens consent.
    Love the truth but pardon error.
[reply]
[d/l]
[select]
      You're very welcome. :-)

      A few notes:

      • The first $depth++ should be $depth--.

        That's a bug in your original posting. I was wondering how $depth decremented ...

      • And $newpos + 2 had to be $pos += $newpos + 2.

        You're absolutely correct - I missed that.

      Being right, does not endow the right to be rude; politeness costs nothing.
      Being unknowing, is not the same as being stupid.
      Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence.
      Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.

[reply]

      Honestly, I did think of doing it that way, but considered it rather too repetitive. The problem is there are conditions under which you want to change the depth for your next state — but a separate issue is whether you want to change depth before processing the current state. That's why the code ends up so messy.

      The following doesn't get rid of the variable, but rather repurposes and renames it to separate the two distinct decisions involved more cleanly.

      {
    my( $indent, $depth ) = ( "\t", 0 );
    sub pp {
        my $next_depth;
        my( $end_chunk, $first ) = f1( $str );
        if( defined $first ) {
            if( $first eq CONST1 ) {
                ++$end_chunk;
                $next_depth = --$depth;
            }
            elsif( defined my $pos2 = f2( $str, $first ) ) {
                $next_depth = $depth;
                $end_chunk = $pos2 + 2;
            }
            else {
                $next_depth = $depth + 1;
            }
        }
        else {
            $next_depth = $depth;
        }

        print $indent x $depth, substr( $str, 0, $end_chunk );
        $str =~ s[^.{$end_chunk}\s*][];
        $depth = $new_depth;
        return;
    }
}

      [download]

      I still think your code is doing a lot of manual pattern matching which could be better done by the regex engine.

      Makeshifts last the longest.

[reply]
[d/l]
        I still think your code is doing a lot of manual pattern matching which could be better done by the regex engine.

        By that, I assume you mean the indexBal() routine.

        If you can encapsulate this into a more efficient, or more simple routine by using the regex engine, please do, it would help me a lot. I tried and failed. 

        sub indexBal {
    my( $string, $lookFor, $limit ) = @_;
    my $nesting = 0;
    for( my( $position, $char ) = ( 0, substr $string, 0, 1 );
            $position < min( length( $string ) , $limit );
            $char = substr $string, ++$position, 1
    ) {
            $nesting++ if $char =~ m/[\[\{]/;
            $nesting-- if $char =~ m/[\]\}]/;
            die 'Unbalanced' if $nesting < 0;
            return $position if $char eq $lookFor and $nesting == 0
    }
    return;
}

my $s = '{ [ { [ { x }, { y } ], [ z ] } ] } { [ { ';

my $pos = indexBal $s, '}', length( $s );

print substr $s, 0, $pos+1;

{ [ { [ { x }, { y } ], [ z ] } ] }

        [download]

        Note that:

      • The brackets and braces may be nested to any arbitrary depth.
      • The requested match may not yet be available.
      • There will nearly always be an incomplete set of nested brackets on the end of the string.
      • The routine takes a parameter that specifies the maximum length in which the match must be found else don't bother looking any further.
        Examine what is said, not who speaks.
        Silence betokens consent.
        Love the truth but pardon error.
[reply]
[d/l]
[select]
Re^2: Refactoring challenge.
by tphyahoo (Vicar) on Mar 08, 2005 at 09:20 UTC
    It seems that $_->print calls $_print... or am I wrong?

    I don't understand this. Is there a name for this feature/behavior of perl?

    Nice refactor++.

[reply]

In Section Seekers of Perl Wisdom