Ok, lets get one thing very clear, right from the start. The code you have written here is shocking. It is bad, awful, it is the kind of code that gives me nightmares in which I get dragged in at the last minute to fix up a previous contractors work only to discover a myriad of perl files lying around, all containing code like this.

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.

---

---

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.