First: use placeholders for all your DBI queries. You're taking in raw data from the CGI and putting that into your SQL queries, and that's a screaming security hole. It's better to use placeholders, as DBI will quote away any offending characters and offer you more protection. That is, instead of :

my $query = "SELECT location FROM demstock2 WHERE ds_id=$main";
my $sth   = $db->prepare($query);
$sth->execute or print "Can't execute <pre>$query</pre>: " . $db->errs
+tr . "<br>\n";
[download]

use

my $query = "SELECT location FROM demstock2 WHERE ds_id=?";
my $sth   = $db->prepare($query);
$sth->execute( $main ) or print "Can't execute <pre>$query</pre>: " . 
+$db->errstr . "<br>\n";
[download]

Whatever was in $main will be appropriately quoted at to make the SQL still valid and to protect your DB.

Now, as for other aspects:

• You have if ( $action ) { then if ($action eq "BOND") I will presume that you have several possible actions and you're only distilling down here. Even in this case you are (correctly) checking for allowed actions, so the cases where $action isn't set and where $action isn't a valid action distill to the same entry. You can get rid of the if ($action) in this case.
• You are trying to provide a lot of descriptive errors: this is good (though some argue how much detail on errors you want to give back to the user); however, this makes you play havoc with lots of embedded if statements. In combination with the $action statement above, it is probably better to do two things: first, create several subroutines that incorporate only the tasks for those actions, eg, you'd have a do_bond sub for $action eq 'BOND'. You can use a hash for storing these:

  my %actions = { 'BOND'=>\&do_bond,
                  'SHARE'=>\&do_share }
  if ( exists $actions{ $action } ) {
     my $err = &{ $actions{ $action } };
     if ( $err ne '1' ) {
        print $err;
     }
  } else {
     print "Action is not defined.";
  }
  [download]

  Now, in the subs, you don't have to print out the error codes; since these errors are 'fatal', simply return the error message; this avoids having to write the else brance, and thus 'flattens' your code more. With the code above, just make sure that successful completion returns '1'. This should now make your logic look like:

  sub do_bond {
     get LOCATION;
     check LOCATION or return;
     foreach @acc
        check PARENT_ID or return;
        update & insert or return;
     foreach @built
        check PARENT_ID or return;
        update & insert or return;
     return 1; # if you got here, everything's fine
  }
  [download]

There also might be some fine-tuning of the SQL to minimize the number of calls to the DB that you use, but without refering to references or knowing what SQL you've got, I can't say for sure.

Update After reading a later reply, I misunderstood the logic in the for blocks; it doesn't change the suggestions above, as another post points out, you can still 'short circuit' with next.:

sub do_bond {
   get LOCATION;
   check LOCATION or return;
   foreach @acc
      check PARENT_ID or { set error; next; }
      update & insert or { set error; next; }
   foreach @built
      check PARENT_ID or { set error; next; }
      update & insert or { set error; next; }
   return 1; # if you got here, everything's fine
}
[download]

In such a case where multiple error statements were collected, I'd not print them all, but would collect them into an array or string, this being returned. An empty array or string would imply success.

IMO I think it's more important to try to avoid shuffling the main logic of a program into the else part of an if block *unless* the 'then' portion has as much or more logic. When dealing with web services in which you want to only handle cases you specifically code for, this may seem difficult to write, but cautious use of 'next', 'last', and 'break' statements can help maintain that. Also, it's just as easy to swap the else and then blocks for any statements, pushing all error handling to the end of the cdoe.

-----------------------------------------------------
Dr. Michael K. Neylon - mneylon-pm@masemware.com || "You've left the lens cap of your mind on again, Pinky" - The Brain
"I can see my house from here!"
It's not what you know, but knowing how to find it if you don't know that's important

---

---

Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!

Titles consisting of a single word are discouraged, and in most cases are disallowed outright.

Read Where should I post X? if you're not absolutely sure you're posting in the right place.

Please read these before you post! —

• How do I compose an effective node title?
• How do I post a question effectively?
• Markup in the Monastery

Posts may use any of the Perl Monks Approved HTML tags:

a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr

You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)

	For:		Use:
	&		&
	<		&lt;
	>		&gt;
	[		&#91;
	]		&#93;

Link using PerlMonks shortcuts! What shortcuts can I use for linking?

See Writeup Formatting Tips and other pages linked from there for more info.