I believe that it is time for what we call in the parenting world a "time out". Step away from the computer... take a deep breath... go for a walk... read a book. Try not posting for about a week and just reading what others here have to say. Then, when you return to your problem, you will be surprised at how much simpler it all seems.

()-()
 \"/
  `                                                     

Rather than duplicate code, you should consider a looping construct, which has the added benefit of simplifying code maintenance when you make any changes (as you'd only need update one piece of code to affect all instances).

foreach my $idx (qw( 7 9 11 )) {
  my $next = $idx + 1;

  if ( $slog[$idx] && $slog[$next] ) {
     (@nslog) = split(/\n/, $slog[$next]);

     print "<br><b>$slog[$idx]:</b> <select name=\"$slog[$idx]\">";

     foreach $line (@nslog) {
       print "<option value=\"$line\">$line</option>";
     }

     print "</select>";
  }
}
[download]

    --k.

You should use strict.pm. Get rid of your globals. Using select * in anything that you don't want to break is silly. Using it with positional logic means that you don't want to understand your code, you don't want anyone else to either. Plus you had better be planning on rewriting it in a hurry at the worst possible time. Because you will have to whenever someone rebuilds the database table and moves columns around on you. Fixing all that is a minimum.

And just in case you have bought into the peculiar stupidity which says that unreadable code provides "job security", it doesn't. Deliberately produce crap, and it will be cheaper for your employer to fire you and rewrite it. The sooner the better so that you have produced less to rewrite. (They admittedly often don't realize this fast enough.)

I don't say this to make you mad, or cry, or think I'm a horrible person (I probably am, but thats another story :). I say this to shock you out of any opinion that you might have had that what you have written there is acceptable.

You need to think, and study, very hard. You need to start doing this right away, because code like that will kill you, or the guy after you.

Overview:

• Failure to understand and use more advanced perl modules (See HTML::Template and others)
• Failure to follow principles of abstraction (use of param() function makes this function useful only within a CGI, and when called by the browser directly)
• Failure to understand and utilise fetchrow_hashref and equivalents within DBI module, causing serious maintenance issues (adding a column could break code)
• Failure to follow web security procedure (placed 'id' value directly into an SQL statement without any form of validation or quoting)
• Failure to account for all conditions (what if the query returns an empty result-set? no such item?)
• Failure to use 'strict','warnings' and tainting
• Failure to use my() to correctly scope variables, thus potentially over-writing other functions' $sth or @slog variables
• Failure to create subs for commonly formed operations

I could go on, but I think I got the major points there. Andrew, if you don't understand any of those, you need to find out, either post a reply here with relevant questions and I'll do my best to answer if I get time, or read elsewhere on this site, and in the perl manpages. Buying an O'Reilly book wouldn't be a bad move either, I'm sure other monks can recommend the correct books for you.

Regarding how it should look, without using additional modules (I recommend HTML::Template amoung others) it should look something like this:

sub cart_preview_conditional_element {                                
+                                      
        my ($key, $value) = @_;                                       
+                                      
        if (!($key && $value)) { return; }
 
        print qq{
                <b>${key}:</b> $value <br/>";
        }
}
 
sub cart_preview_conditional_selection {
        my ($key, $value) = @_;
        if (!($key && $value)) { return; }
 
        my @options = split(/\n/,$value);
        print qq{
                <b>$key</b>:
                <select name="$key">
        };
        for (@options) {
                print qq{
                        <option value="$_">$_</option>
                };
        }
        print qq{ </select> };
}
 
# Takes database handle and cart item ID
sub cart_previewitem {
        my ($dbh, $id) = @_;

        if ($id !~ /^\d+$/) { die "Invalid ID (not a number)"; }

        my $sth = $dbh->prepare("select * from items where id='?'");
        $sth->execute($id) || die $dbh->errstr;

        my $row = $sth->fetchrow_hashref() || die "No such item";

        print qq{
                <div class="item_preview">
                        <h2>$row->{'product'} - $row->{'title'}</h2>
        };

        if ($row->{'image'}) {
                print qq{
                        <div class="item_preview_image">
                                <img src="$row->{'image'}"/>
                        </div>
                };
        }
         
        print qq{ <div class="item_preview_details"> };

        cart_preview_conditional_element("Description",$row->{'descrip
+tion'});
        cart_preview_conditional_element("Size",$row->{'size'});
        cart_preview_conditional_element($row->{'misc_key_1'},$row->{'
+misc_value_1'});
        cart_preview_conditional_element($row->{'misc_key_2'},$row->{'
+misc_value_2'});

        print qq{ </div> };

        cart_preview_conditional_selector($row->{'selection_name_1'}, 
+$row->{'selection_values_1'});
        cart_preview_conditional_selector($row->{'selection_name_2'}, 
+$row->{'selection_values_2'});

        print qq{ </div> };

};
[download]

Although that isn't tested code, and I went way overboard on the stylesheet stuff (its SO much nicer to write and read) and to be honest, HTML::Template would make the whole thing much prettier anyway.

Further note: there are situations even within this code in which it will fail. Notably, the instance in which the misc_value or selection_value field intentionally contains a 0. In these instances, the fact that the field is a 0 will fail the $key && $value test and the widget will not be displayed. A specific test may be needed ( && ne "0" or something) since in theory the element is defined even if the field is empty.