Second, you should be using the DBI quote method or placeholders (my preference is placeholders), instead of pasting your variables directly into the sql statement. Placeholders would save you from all that backwhacking of quotes, and in some databases you'd use single instead of double quotes to quote the args, so if you ever migrated, it'd save one extra headache. Placeholders don't get you alot efficiency-wise in mysql, its true, but in combination with prepare_cached can save a bit in statement handle creation (of course then the dbh would have to be persistent also using connect_cached or a package global or as an object attribute or something...).

Third, I think its kind of pointless to copy all those parameters to local args, just to paste them into the sql statement. I'd do something like the insert_hash example in the DBI docs. If this InsertProspect method is already in your main Prospect (or whatever you're calling it) class/package, then I'd not use the accessor methods and just use a hash slice (assuming your object IS a hash underneath) to get all the names and values for the insert statement.

If you can update the prospect object's methods, I would recommend adding a method that will add the prospect to the database for you. Another solution would be a method that returns all of the data for you in an array or hash. Barring that, the following really ugly hack could work:

    {
        # Get the new prospect
        my $prospect = shift;
    
        # Connect to the database
        my $dbh = DBI->connect('DBI:mysql:menagerie', 'menagerie') 
            or die "Couldn't connect to database:" . DBI->errstr;
    
        # Set all of the variables that we need to pass to MySQL
        my @data = qw/ name      address    address2      state      c
+ity 
                       phone     phone2     phone3        phone4 
                       phoneType phone2Type phone3Type    phone4Type
                       email     url        interestLevel designFirm 
                       hostingFirm companyName /;
        my @insert_data;
        my $place_holders = '?,' x @data;
        chop $place_holders; # remove trailing comma
    
        foreach ( @data ) {
            no strict;
            my $method = $data[$_];
            push @insert_data, $prospect->$method;
        }
    
        # Insert the new prospect in the database
        my $sth = $dbh->prepare("INSERT INTO prospect VALUES ( $place_
+holders )" );
        # Execute the statement
        $sth->execute( @insert_data ) or die print DBI->errstr;
    
        # Lets get the prospect id  that we just added back
        my $ID = $dbh->{'mysql_insertid'};
        return $ID;
    }
[download]

The problem, of course, is when you turn off strict for the method calls, you run the risk of unintentionally calling the wrong method if you misspelled it. Further, if you have inherited the misspelled method, you could be in big trouble. The solution would be to write an autoload routine that detects whether or not the method exists and croaks rather than allowing you to inherit something. Of course, if you can write the autoload method, you could write the other methods :)

If you can't change the prospect methods, subclass it and add your own method. Much safer that way!

Other comments: you don't have a password in your connect string. If you left it off because you don't want us to see it, good for you! If you left it off because there is no password, shame on you! :)

I also noticed that you have repetitive phone data. As soon as someone has a fifth phone, your schema breaks. If you need to have multiple phones, create a separate table for them.

Cheers,
Ovid

Vote for paco!

Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.

my $sth = $dbh->prepare("INSERT INTO prospect (name, address, address2
+, state, 
                         city, [snip]) VALUES (?,?,?,?,?,[snip])";

$sth->execute($prospect->name,$prospect->address,$prospect->address2,[
+snip]) 
    or die print DBI->errstr;
[download]

-Blake

  # Save the prospect object

  eval { $prospect->save };
  if ( $@ ) {
      die "Cannot save prospect:",
          "$SPOPS::Error::system_msg\n";
  }
  print "Prospect ID: ", $prospect->id, "\n";
[download]

Chris
M-x auto-bs-mode

After spending sometime with the other perl monks here is my revised version of the same function above... please let me know what you think

sub InsertProspect
{
    # Get the new prospect
    my $prospect = shift; 
    
    # Connect to the database
    my $dbh = DBI->connect('DBI:mysql:menagerie', 'menagerie') or die 
+DBI->errstr;

    # Insert the new prospect in the database
    my $sth = $dbh->prepare("INSERT INTO prospect VALUES (NULL, ?, ?, 
+?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)");
    
    # Execute the statement
    $sth->execute($prospect->name, $prospect->address,
                  $prospect->address2, $prospect->state,
                  $prospect->city, $prospect->phone, 
                  $prospect->phone2, $prospect->phone3,
                  $prospect->phone4, $prospect->phoneType,
                  $prospect->phone2Type, $prospect->phone3Type,
                  $prospect->phone4, $prospect->email,
                  $prospect->url, $prospect->interestLevel,
                  $prospect->designFirm, $prospect->hostingFirm,
                  $prospect->companyName) or die DBI->errstr;

    # Lets get the prospect id  that we just added back
    $sth = $dbh->prepare("SELECT last_insert_id() from prospect");
    $sth->execute or die DBI->errstr;
    my @id = $sth->fetchrow_array;
    my $ID = $id[0];

    # Return the new prospects ID
    return $ID;
}
[download]

-- rogueFalcon

Why do you people insist on doing things sdrawkcab?

It is a really bad idea to rely on knowing the order of columns in a database. You have a table with 20 columns. You have an insert with 20 values. If the order is off slightly, have fun debugging! As columns are added or dropped, this will be a constant source of bugs. (Just imagine if some future DBA has to do a database migration and happens to reorder the columns. Then your scripts all have to work against that..?)

This is typically an issue any time you use positional based logic. Associating things by name works much better.

Therefore if the code is not performance critical, I would strongly suggest storing the data structure to insert in an anonymous hash, and then dynamically construct the insert statement from that, walking the keys to get the field names, and values to get the values, and using the syntax:

my $sql = qq(
  insert into foo (bar, baz)
  values (?, ?)
);
[download]