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~