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.

---

---

• Are you posting in the right place? Check out Where do I post X? to know for sure.
• Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:

  <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>

• Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
• Want more info? How to link or How to display code and escape characters are good places to start.