[sprog-users] Re: Sprog::Gear::ReadExcel.pm Code Review

[Marc S. <mar...@on...>]

Grant McLean wrote:

>I've also been working through your ReadExcel.pm code and coming up with
>some suggestions for making it both better and simpler.  I hope the
>suggestions don't come across as critical because they're not intended
>to be.  I really do appreciate the fact that you've slogged through my
>code enough to get something working.  Looking at your code has given me
>some ideas for how I can make things easier for gear-writers (including
>me).
>
>* I'd recommend moving the 'use strict' and related lines to after
>  the metadata block.  When Sprog is scanning @INC for gear classes
>  it 'parses' the start of each .pm file.  Any extraneous code
>  between the package name and the metadata section runs the risk of
>  confusing the simple parser.
>
>  
>
This was an oversight by me.  Normally I always use this, but since I 
was in "Play Mode" with the module I didn't go too far down the path of 
making it super-tight.  A rework of this code already going on in the 
background.

>* I'm guessing you based your code on the TextInput gear and some
>  bits of code are artifacts from that.  For example you declare a
>  property called 'text' which isn't really needed; an accessor
>  called gear_view that isn't used at all; and an accessor called
>  filename that really ought to be a property.  
>
>  
>
Yes, I did start that way.  Actually, the first thing I tried was to 
"use base qw(Sprog::Gear::ReadFile)" and override the _open_file 
subroutine.  I got that to work, but couldn't get the IO::Scalar fake 
filehandle to work.  After that I went with the TextInput gear since its 
straightforward.  There are probably several strange bits of code that 
don't belong there. I figure that as I get better with the framework 
I'll start writing code that is simpler.

>
>* The file_start and file_end events both send an argument of undef.
>  It really ought to be the filename.
>  
>
Next version will do this.

>* As Tony Bowden observed, your gear probably shouldn't be outputting
>  CSV data.  The 'List' output connector is probably a better choice.
>  People can always use the ListToCSV gear if that's what they want.
>  To use the list connector, just change the value for 'type_out' in
>  the metadata from 'P' to 'A', then instead of calling:
>  
>    $self->msg_out(data => $self->text);
>
>  to send all the CSV data, call:
>
>    $self->msg_out(row => \@cell_values);
>
>  once for each row in the spreadsheet - regardless of whether it's a
>  header or not.
>  
>
>* I'd also suggest that your gear shouldn't read from a file at all.
>  Instead, it should have a type 'P' input connector and simply
>  accumulate all 'data' events.  Then in the file_end event it can
>  call $XL->Parse(), passing it a reference to a scalar containing
>  all the accumulated data instead of a filename.  See ParseHTMLTable.pm
>  for example code.
>  
>
I thought of that as soon as I sent the link last night.  No problems, I 
will update the code to handle input this way.

>
>  Another advantage is that your gear would be simpler.  It wouldn't
>  need the Text::CSV_XS dependency, or the routine for accepting
>  drag-n-drop and it would even need a dialog box at all.  Unless you
>  decided to implement Tony's suggestion of allowing the user to select
>  which sheet to parse.  In which case, I'd suggest a dialog like the
>  ParseHTMLTable gear - one simple entry box where the user can type
>  either the sheet number or the name.
>
>  
>
I would love to implement this.  I'll probably be needing it soon 
anyway.  I have one question about the XML dialogs.  It my be 
documented, and if it is tell me to RTFM.  I would like to have the 
system pop up the dialog box with a pulldown list of the sheets on the 
excel file if they have not selected one.  I know how to make the 
dialog, but didn't know if there was an event I could call to pop up the 
dialog myself from the gear.  I'm not sure its even the best way to do it.

Perhaps even another way would be to put in a gear that splits the excel 
file into a list connector, one sheet for each record.  Any thoughts on 
the preferred methodology?

I don't take any of these things critically, I'm always looking to learn 
something new or improve something I'm working on.  After this excel 
module is put to rest I plan on moving into an XSLT gear, since I 
already have a need for that.  I'm also considering touching MySQL data 
input/output gears, since being able to shuffle spreadsheets on my 
desktop and either read or write records to my database would also make 
life a lot easier.

Thanks!
Marc