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

In this node, I posted some code for review. I also asked (both in the node and in CB) for advice on transforming it to use an Object-Oriented style. (Please note that this was primarily an exercise in using OOP for what -- to me -- did not seem like a good candiate for OOP.)

After getting a little direction from various monks, here is my first draft Thanks to jeffa for helping me get started, and demerphq for warning me off of this project. :-)

In focussing on the OO structure, I did not include all of the functionality of the original version. There are also some things that I've already noticed that I need to work on (such as commenting, error handling and the possibly unnecessary connection between the two objects), but I wanted to get feedback as quickly as possible.

As before, please feel free to criticize style, efficiency, etc. Thanks in advance for anything you can do to help me improve my programming skills and knowledge.

#!/usr/bin/perl -w # db_import.plx # by Brad Smithart (AKA Impossible Robot) # OOP version # $Revision: 1.5 $ # Uses tab-separated data file with header column use strict; my $exh = new Data::Reader( filename => 'myfile.txt', _debug => 1 ); $exh->connect() or croak $exh->error; print "Columns: ",join(', ', $exh->columns), "\n"; print "# of Cols: ", scalar($exh->columns), "\n"; my $imp = new Data::Import( reader => $exh, dsn => 'DBI:mysql:exhibit:localhost', username => 'user', password => 'pass', _debug => 1 ); $imp->connect() or die $imp->error; my @values; while (@values = $exh->row) { $imp->insert(@values); } $exh->disconnect(); $imp->disconnect(); exit; ###################################################################### # Classes ###################################################################### package Data::Reader; use Carp; sub new { my $pkg = shift; bless { _error => 'No error', _connected => 0, _debug => 0, @_ }, $pkg; } sub connect { my $self = shift; my %attr = @_; # Connect string params can override constructer params for (keys %attr) { $self->{$_} = $attr{$_}; } print "Data::Reader Connect:\n" if $self->{_debug}; eval { # Open file open(FILE, $self->{filename}) or croak "Data::Reader couldn't connect: $!"; print " Filename: ", $self->{filename},"\n" if $self->{_debug}; $self->{_handle} = *FILE; # Get field names from header chomp(my $header = readline($self->{_handle})); $header =~ tr/"//d; $self->{_fieldnames} = [ split(/\t/, $header) ]; @{ $self->{_fieldnames} } or croak "Data::Reader - No valid header"; $self->{_connected} = 1; }; if ( $@ ) { $self->{_error} = $@; $self->{_connected} = 0; return 0; } return 1; } sub disconnect { my $self = shift; close(FILE) or return 0; $self->{_connected} = 0; print "Data::Reader Disconnect.\n" if $self->{_debug}; return 1; } # Accessor methods sub columns { my $self = shift; unless ($self->{_connected}) { carp ("Data::Reader - No connection\n"); return; } return @{ $self->{_fieldnames} }; } sub row { my $self = shift; my $row = readline($self->{_handle}); return unless $row; chomp($row); $row =~ tr/"//d; my @values = split(/\t/, $row); return @values; } sub connected { my $self =shift; $self->{_connected}; } sub error { my $self = shift; $self->{_error}; } sub filename { my $self = shift; if (@_) { $self->{filename} = shift }; return $self->{filename}; } ###################################################################### package Data::Import; use DBI; use Carp; sub new { my $pkg = shift; bless { dsn => "DBI:mysql:database:localhost", username => 'root', password => '', table => 'exhibit', reader => undef, _error => 'No error', _debug => 0, @_ }, $pkg; } sub connect { my $self = shift; my %attr = @_; for (keys %attr) { $self->{$_} = $attr{$_}; } print "Import Connect:\n" if $self->{_debug}; eval { $self->{dsn} or croak "Data::Import - No DSN"; my $dsn = $self->{dsn}; my $username = $self->{username}; my $password = $self->{password}; $self->{_database} = DBI->connect($dsn, $username, $password ) or croak $DBI::errstr; print " Connect string: ", $self->{dsn}, ',', $self->{username}, ',', $self->{password}, "\n" if $self->{_debug}; $self->{reader} or croak "Data::Import - No Reader"; my $reader = $self->{reader}; unless ($reader->{_connected}) { $reader->connect() or croak $exh->error; } my $insert_fields = join(', ', $reader->columns); my $placeholders = join(', ', ('?') x $reader->columns); my $table = $self->{table}; my $query = <<"EOQ"; INSERT INTO $table ( $insert_fields ) VALUES ( $placeholders ) EOQ $query =~ tr/\r\n//d; $query =~ s/\s{2,}/ /g; $query =~ s/^\s|\s$/ /g; print " Query: $query\n" if $self->{_debug}; $self->{_query} = $self->{_database}->prepare($query); }; if ( $@ ) { $self->{_error} = $@; return 0; } return 1; } sub disconnect { my $self = shift; $self->{_database}->disconnect(); print "Import Disconnect.\n" if $self->{_debug}; } sub DESTROY { shift->disconnect(); } sub insert { my $self = shift; $self->{_query}->execute(@_); print "Execute: ", join(',', @_), "\n" if $self->{_debug}; } # Accessor methods sub dsn { my $self = shift; if (@_) { $self->{dsn} = shift }; return $self->{dsn}; } sub username { my $self = shift; if (@_) { $self->{username} = shift }; return $self->{username}; } sub password { my $self = shift; if (@_) { $self->{password} = shift }; return $self->{password}; } sub table { my $self = shift; if (@_) { $self->{table} = shift }; return $self->{table}; } sub error { my $self = shift; $self->{_error}; }

Impossible Robot

Replies are listed 'Best First'.
Re: For Review: OOP DB Import
by dash2 (Hermit) on Feb 05, 2002 at 00:16 UTC

    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); } }

    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~

      Thanks, dash2, for your critique. If you haven't had a chance to look at my previous post, it might help explain some of my reasoning -- or it may not. :-)

      1) It think Text::CSV_XS would be overkill for this data, which only takes two lines of code to parse.

      2) You are absolutely right about the coupling between the two classes. I need to 'fish or cut bait.' I'm not really committed to either tight-coupling or de-coupling. I considered passing in an array ref containing the column names, but I think I'll go with your suggestion and build an 'insert all' method. (Of course, the 'insert' method is not entirely generic, since it executes a query prepared in the 'connect' method.)

      3) There are several reasons I did not want to simply 'die' inside my methods -- for example, if I was processing multiple files and wanted to simply record any failures but continue processing with the next file. I could move the 'eval' outside of the object, but I felt like encapsulating these kinds of details was an OO advantage. But I would definitely like to discuss the pros and cons of various types of error-handling.

      4) Any suggestions on what a 'debug' method should look like? It sounds like a great idea.

      Thanks again for helping me rethink this.

      Impossible Robot
Re: For Review: OOP DB Import
by lachoy (Parson) on Feb 04, 2002 at 23:43 UTC

    One slightly tangential note: using a module like Class::Accessor automates the accessor/mutator method creation for you, and makes it simple to add new ones as well:

    package My::Object; use strict; use base qw( Class::Accessor ); my @FIELDS = qw( dsn username password table ); My::Object->mk_accessors( @FIELDS ); ...

    To add a new accessor/mutator, we just need to stick a new field into @FIELDS.

    Chris
    M-x auto-bs-mode

      Thanks, lachoy. I've seen several pieces of code (including a couple of examples here at the Monastery) for auto-generating accessors using closures (which is apparently what Class::Accessor does). But I had never seen this module before; I think it might be useful.

      I didn't auto-generate the accessors in this first draft because I wasn't sure of the design -- specifically what accessors I should use and how they should work. But that was stupid, because I could have shortened the code somewhat and made it just as easy to modify later.

      (One of the purposes for this exercise was to use the language I know -- and love -- best to master concepts from languages I don't know. Do other languages have the ability to auto-generate accessors?)

      Impossible Robot