I have the following pretty complex Perl code that is fed by a dynamic array varying in size but typically between 10 and 20 rows. I'm therefore running this in a loop but reckon there must be a more elegant way of inserting the data.
Is it possible to improve this code to make it more efficient?
sub data_fill {
my $target = shift;
my (@input_data) = @_;
# Delete the old data
my $sth = $dbh->prepare("DELETE FROM work_data WHERE target = '$target' ");
$sth->execute();
# Insert the new data
my $sql = qq{INSERT INTO work_data (target, trackingno, temp, apple, orange, banana, strawberry, lettuce, apricot, peach, blackberry, melon, lemon) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?)};
my $sth = $dbh->prepare($sql);
my $arraysize = scalar(@input_data);
my $i = 0;
while ($i < $arraysize) {
$sth->bind_param(1, $target, SQL_VARCHAR);
$sth->bind_param(2, 'OK', SQL_VARCHAR);
$sth->bind_param(3, $input_data[$i], SQL_VARCHAR);
$sth->bind_param(4, $input_data[$i + 1], SQL_VARCHAR);
$sth->bind_param(5, $input_data[$i + 2], SQL_VARCHAR);
$sth->bind_param(6, $input_data[$i + 3], SQL_VARCHAR);
$sth->bind_param(7, $input_data[$i + 4], SQL_VARCHAR);
my $lettuce = $input_data[$i + 5];
my $apricot = $input_data[$i + 6];
my $peach = $input_data[$i + 7];
if (length($lettuce) == 0) {
$lettuce = -1;
} # Safety net to help later in db sorting
if (length($apricot) == 0) {
$apricot = -1;
} # Safety net to help later in db sorting
if (length($peach) == 0) {
$peach = -1;
} # Safety net to help later in db sorting
$sth->bind_param(8, $lettuce, SQL_INTEGER);
$sth->bind_param(9, $apricot, SQL_INTEGER);
$sth->bind_param(10, $peach, SQL_INTEGER);
$sth->bind_param(11, $input_data[$i + 8], SQL_VARCHAR);
$sth->bind_param(12, $input_data[$i + 9], SQL_VARCHAR);
$sth->bind_param(13, $input_data[$i + 10], SQL_VARCHAR);
$sth->execute;
$i = $i + 11; # INCREMENT
}
$dbh->disconnect;
return;
}
Your code is not so much complex as convoluted.
The first thing I see is that $dbh->prepare is called every time the subroutine executes. Worse still, there is a $dbh->disconnect before the return.
I would also look at the binding, which could be permanent if you copied the parameters to a static array instead of binding them all every time.
Update
This is to try to explain what I mean. The code inside the unless statement will be executed only once - the first time the data_fill subroutine is called. It prepares the delete and insert statements, and binds the @params array to the place holders in the insert statement.
Note that the prepare and bind_param calls don't have to be within this subroutine. They are really part of initialisation, and the unless is there to make sure that they are executed only once.
Once all that has been set up, all that remains is to execute the delete statement, and then repeatedly fill the @params array from the subroutine parameter list and execute the insert statement. I have removed the $dbh->close altogether as it's not the right place to be closing the database handle.
This solution has far too many "magic numbers" in it, and I wouldn't publish it as it stands, but I don't know enough of your requirement totell whether I'm even on the right lines.
Note that none of this is tested, beyond checking that it will compile
my ($delete, $insert, @params);
sub data_fill {
my $target = shift;
my (@input_data) = @_;
unless ($delete and $insert) {
$delete = $dbh->prepare('DELETE FROM work_data WHERE target = ?');
$insert = $dbh->prepare('
INSERT INTO work_data (
target, trackingno, temp, apple, orange, banana,
strawberry, lettuce, apricot, peach, blackberry, melon, lemon
)
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
');
for my $i (0 .. 10) {
my $mode = SQL_VARCHAR;
$mode = SQL_INTEGER if $i >= 7 and $i <= 9;
$sth->bind_param($i+1, $params[$i], $mode);
}
}
$delete->execute($target);
while (@input_data >= 11) {
my @record = splice @input_data, 0, 11;
@params = ($target, 'OK', @record);
for (@params[7..9]) {
$_ = -1 unless length > 0;
}
$insert->execute;
}
}
Collected from the Internet
Please contact [email protected] to delete if infringement.
Comments