I'll have a shot.

1. I guess the same applies to you as to the guy who got soundly beaten for suggesting that his method for reading http parameters was "as good as" CGI.pm... are you sure you shouldn't be using an existing solution like Text::CSV_XS? I merely ask. If they don't do what you want, you could even subclass them, which is a good test of OO skills.

2. Your two packages are "tightly coupled" - they talk to each other a lot. For example Data::Import's reader attribute has to have a connect() and columns() method. But then you have an insert() method which totally ignores all this and could be used to insert any data. You've gone to the trouble of setting up this very specific SQL statement; so why not couple the insert() method too, to avoid it being misused? 

sub import {
 my $self = shift;
 my @data;
 while (@data = $self->{reader}->row) {
  $self->{_query}->execute(@data);
 }
}

[download]

The alternative is to couple things more loosely, so your Data::Import::connect() method would be less specific about columns etc. But then you might well think, 'why not just use DBI?'

3. Not an OO point, but: why the big evals? Sometimes it is better to just die. Otherwise you have to keep error checking. Unless you have a reason to keep the program going, you may as well admit there is a problem, and tell the user by dying. (But maybe you do have a reason.)

4. You might as well chuck in a debug() method, that way if you want to debug in a different way (to a file, STDERR, or by dying) you will only have to change things in one place. You could export it from a utility class... I expect there is stuff on CPAN but for this size of project writing it yourself probably is just as easy... until the project grows, anyway :-)

Overall it is pretty good and better than my first OO attempts. I like having two separate objects, you will be able to do different things with your data having read it, and that will be useful.

dave hj~

In reply to Re: For Review: OOP DB Import by dash2
in thread For Review: OOP DB Import by impossiblerobot

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":


  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.