xiaoyafeng has asked for the wisdom of the Perl Monks concerning the following question:

Hi monks

Recently, I've been writing a excel template program based on Win32::OLE. for reusing and code elegance, I've used a callback like what File::Find does. Below is sketch of framework: 

-----------       ------------           -------------    
|excel     |     |           |           |           |
|template  | <== | callback  |   <====   |database   |       
|          |     |           |           |___________|
|          |     |           |
---------        |___________|   <=====  -------------
                                         |            |
                                         |DCOM server |
                                         |            |
                                         --------------

[download]
below are some part of codes: 
sub cell_walk{
    my $self = shift;
    my ($sheet_name, $x, $y, $callback) = @_;
       
       my $Sheet = $self->{ 'book_handle' }->Worksheets($sheet_name);
    for ( $row = $x->[0] ; $row <= $y->[0] ; $row++ ) {
        for ( my $col = $x->[1] ; $col <= $y->[1] ; $col++ ) {
              $callback->($Sheet->Cells( $row, $col ));
        }
    }

}

sub callback{
     
    return if !defined $_[0]->{Value};
        if ( $_[0]->{Value} =~ /^~~~/ ) {
            my @a =
              split( /__/, substr( $_[0]->{Value}, 3 ) );  #grab usefu
+l string
              if( scalar @a == 3){
            $_[0]->{Value} =
              $hl->get_single_LP( 'ADAS_VAL_RAW', @a );
              }
              if( scalar @a == 4){
            $_[0]->{Value} =
              $hl->accu_LP( 'ADAS_VAL_NORM', @a );
              }
        }


}

[download]
Please note $hl in above code, it is a ref from another big object including DBI and Dcom server handles. at this stage, I new it as a global variable and use it in subroutine directly. it make work done but looks ugly.

So Is there any good way to re-organize code to make my source more pretty? Do I need to pass the ref of the obj to callback in advance? Thanks in advance!!!!

UPDATE: correct codes.



I am trying to improve my English skills, if you see a mistake please feel free to reply or /msg me a correction

Replies are listed 'Best First'.
Re: how organize code
by Khen1950fx (Canon) on Nov 29, 2011 at 07:20 UTC
    For the callback, I did &$callback(. Here's a pretty-print: 
    #!/usr/bin/perl

use strict;
use warnings;

sub cell_walk {
    my $self = shift @_;
    my ( $sheet_name, $x, $y, $callback ) = @_;
    my $Sheet = $$self{'book_handle'}->Worksheets($sheet_name);
    foreach $row( $row = $$x[0] ; $row <= $$y[0] ; ++$row ) {
        for ( my $col = $$x[1] ; $col <= $$y[1] ; ++$col ) {
            &$callback( $Sheet->Cells( $row, $col ) );
        }
    }
}

sub callback {
    return if not defined $_[0]{'Value'};
    if ( $_[0]{'Value'} =~ /^~~~/ ) {
        my (@a) = split( /__/, substr( $_[0]{'Value'}, 3 ), 0 );
        if ( scalar @a == 3 ) {
            $_[0]{'Value'} = $hl->get_single_LP( 'ADAS_VAL_RAW', @a );
        }
        if ( scalar @a == 4 ) {
            $_[0]{'Value'} = $hl->accu_LP( 'ADAS_VAL_NORM', @a );
        }
    }
}

    [download]
    Does that help?
[reply]
[d/l]
[select]
Re: how organize code in callback mode
by roboticus (Chancellor) on Nov 29, 2011 at 11:41 UTC

    xiaoyafeng:

    I like the version that Khen1950x, but as you suggest in your last question, I'd also pass $hl to the callback function so you don't have to rely on the global. Here's his version with the (untested) change:

    #!/usr/bin/perl

use strict;
use warnings;

sub cell_walk {
    my $self = shift @_;
    my ( $sheet_name, $x, $y, $callback, $callback_data ) = @_;
    my $Sheet = $$self{'book_handle'}->Worksheets($sheet_name);
    foreach $row( $row = $$x[0] ; $row <= $$y[0] ; ++$row ) {
        for ( my $col = $$x[1] ; $col <= $$y[1] ; ++$col ) {
            &$callback( $Sheet->Cells( $row, $col ), $callback_data );
        }
    }
}

sub callback {
    # $_[0] = excel cell reference, $_[1] = XYZ object reference
    return if not defined $_[0]{'Value'};
    if ( $_[0]{'Value'} =~ /^~~~/ ) {
        my (@a) = split( /__/, substr( $_[0]{'Value'}, 3 ), 0 );
        if ( scalar @a == 3 ) {
            $_[0]{'Value'} = $_[1]->get_single_LP( 'ADAS_VAL_RAW', @a 
+);
        }
        if ( scalar @a == 4 ) {
            $_[0]{'Value'} = $_[1]->accu_LP( 'ADAS_VAL_NORM', @a );
        }
    }
}

    [download]

    ...roboticus

    When your only tool is a hammer, all problems look like your thumb.

[reply]
[d/l]

      Thanks for your reply. this version of code looks better, but now, I suspect i am on the right way. In order to avoid exposing variables in global, I have to pass 5 paras to cell_work function. and as a module, $callback_data does confuse end user.

      to my primitive intention, I just want to separate code into subs and modules for managing. But it now seems to raise complexity of code. do I have to come back to below tedious code? 

      
# main, use below to substitute cell_walk and callback routine

for ( my $row = 1 ; $row <= $LastRow ; $row++ ) {
    for ( my $col = 1 ; $col <= $LastCol ; $col++ ) {
        next if !defined $Sheet->Cells( $row, $col )->{Value};
        if ( $Sheet->Cells( $row, $col )->{Value} =~ /^~~~/ ) {
            my @a =
              split( /__/, substr( $Sheet->Cells( $row, $col )->{Value
+}, 3 ) );  #grab useful string
              if( scalar @a == 3){
            $Sheet->Cells( $row, $col )->{Value} =
              $hl->get_single_LP( 'ADAS_VAL_RAW', @a );
              }
              if( scalar @a == 4){
            $Sheet->Cells( $row, $col )->{Value} =
              $hl->accu_LP( 'ADAS_VAL_NORM', @a );
              }
        }
    }
}

      [download]


      I am trying to improve my English skills, if you see a mistake please feel free to reply or /msg me a correction

[reply]
[d/l]
        I would say yes. You can break the loop whenever you want, and you have access to all the lexicals, and therefore can jump cells if 1 cell relies on data on column to left or right of it or on another sheet . If its code that only you will ever use (or your successor) will use, it doesn't matter since you can add new parameters to your callback or get rid of the callback whenever you want and you understand how it works, but if this is an API that is to be used without having to look at its source code, don't use callbacks. There is nothing wrong with the large loop that doesn't use callbacks. Put a comment on the block ending curly bracket line if your editor doesn't show the whole thing at once or your confused by long conditional trees. If you want code organization, your callback becomes a good old plain sub/function when you don't pass sub reference to the caller looping sub and hard code the cell processing sub in. This is assuming your writing a perl app/script and not writing a reusable API.
[reply]
Re: how organize code
by patcat88 (Deacon) on Nov 29, 2011 at 08:01 UTC
    Unless its an event loop with sleeping between events, callbacks are terrible. The parameters your callback is called with is never documented in POD. How can you interleave data from 2 callback based APIs without saving it in a global? What if you found the data you need on the 50th callback run, and you don't want the caller API to run you 10000 more times? How do you quit the callback enumeration loop? Function based enumerations (start() then get() #X times then end() ) are better. Or a batch get() that gets how ever many records you want to swallow in one shot (20 at a time for example). Callbacks are evil unless you have an event loop that kernel sleeps between events. KISS. I find APIs that return flags are much better documented and easier to use than APIs where you register a callback for each flag to run when the enumeration system sees it. Imagine if DOM didn't exist and all browsers only have SAX engines to manipulate the page tree HT/XML. This post isn't specific to you, just all callback APIs.
[reply]

Back to Seekers of Perl Wisdom