Script for updating an Oracle database

Question

I have the following piece of Perl script for updating an Oracle database. It's working perfectly fine, but I want to know how I can simplify this script.

use DBI;
my $dbh;
my $bad_form = "text.txt";
open (OUT, $bad_form) or die "Could not open $bad_form";
while (<OUT>){
 chomp $_;
 my ($item_id, $description, $form_id_1, $form_id_3) = split(/\|/, $_);
 my $new_form_id_1 = form_1($form_id_1);
 my $new_form_id_3 = form_3($form_id_3);
 if ( $new_form_id_1 ne 'null' ){
 my $sql = "update item_table set form_id_1 = '$new_form_id_1' where item_id = '$item_id'";
 my $item = $dbh->prepare($sql) or (log_error("Couldn't prepare statement: " . $dbh->errstr) and return 2);
 $item->execute() or (log_error("Couldn't execute statement: " . $item->errstr) and return 2);
 }else{
 $form_id_1 = "correct value";
 $new_form_id_1 = "no updated";
}
 if ( $new_form_id_3 ne 'null' ){
 my $sql = "update item_table set form_id_3 = '$new_form_id_3' where item_id = '$item_id'";
 my $item = $dbh->prepare($sql) or (log_error("Couldn't prepare statement: " . $dbh->errstr) and return 2);
 $item->execute() or (log_error("Couldn't execute statement: " . $item->errstr) and return 2);
 }else{
 $form_id_3 = "correct value";
 $new_form_id_3 = "no updated";
 }
}

Answer 1

Some basic refactoring, and using ? placeholders in sql queries,

use strict;
use warnings;
use DBI;
my $dbh;
my $bad_form = "text.txt";
sub prepare_execute {
 my ($sql, $values, $dbh) = @_;
 my $item = $dbh->prepare($sql) or do {
 log_error("Couldn't prepare statement: " . $dbh->errstr);
 die;
 };
 $item->execute(@$values) or do {
 log_error("Couldn't execute statement: " . $item->errstr);
 die;
 };
 return 1;
}
open (my $OUT, "<", $bad_form) or die "$! $bad_form";
while (<$OUT>) {
 chomp $_;
 my ($item_id, $description, $form_id_1, $form_id_3) = split(/\|/, $_);
 my $new_form_id_1 = form_1($form_id_1);
 my $new_form_id_3 = form_3($form_id_3);
 if ( $new_form_id_1 ne 'null' ){
 my $sql = "update item_table set form_id_1 =? where item_id =?";
 eval { prepare_execute($sql, [$new_form_id_1, $item_id], $dbh) }
 or return 2;
 }
 else {
 $form_id_1 = "correct value";
 $new_form_id_1 = "no updated";
 }
 if ( $new_form_id_3 ne 'null' ) {
 my $sql = "update item_table set form_id_3 =? where item_id =?";
 eval { prepare_execute($sql, [$new_form_id_3, $item_id], $dbh) }
 or return 2;
 }
 else {
 $form_id_3 = "correct value";
 $new_form_id_3 = "no updated";
 }
}

Answer 2

Happy things

• Thanks for use strict. That is smart.
• I'm also a fan of the 3-argument form of open. That can avoid a few subtle bugs/holes.
• Using a scalar for your filehandle.

Suggestions

Some areas which could be improved:

use perltidy

Your indentation is inconsistent. Perltidy will reformat your code for you with a variety of customization options so it can look as you prefer. Here are my settings if you'd like a starting point. (Tabs and cuddled elses ftw, but ymmv!)

use single quotes for most literals

If you are using a literal that doesn't require anything special like interpolation of variables it is safer to put it in single quotes. For instance I used to see a lot of

 my $email = "[email protected]";

Where perl goes and gets confused because there is no @example array defined (hopefully). SO by using single quotes which don't interpolate or do anything else surprising:

 my $email = '[email protected]';

using placeholders for building SQL queries

I spent a lot of time hand-molding SQL queries in Perl, but it is ultimately safer to use placeholders for your queries as mpapec demonstrated. This will avoid SQL injection attacks and other types of crashes.

simplification

We don't see the definition of form1() and form3() but if they're under your control you could have them return more useful information.

• One way would be to have it return 'correct value' and then branch on that.

• Another way and my preference would be to have it update both values like so:

  #!/usr/bin/perl
  use strict;
  use warnings;
  sub form1 {
   my ($id) = @_;
   if ($id < 10) {
   return ('not updated', 'correct value');
   } else {
   return ($id*10, $id);
   }
  }
  foreach my $form_id (1..20) {
   print "$form_id:\n";
   my $new_form_id;
   ($new_form_id,$form_id) = form1($form_id);
   unless ($new_form_id eq 'not updated') {
   print "in unless\n";
   }
  }