I'm validating user input through forms via CGI using the following script I wrote:

#!/usr/bin/perl -wT

use strict;

use CGI;

my $q = new CGI;

# keys are param names
# values are regexes matching allowed content and max allowed length
my %required_params  = ( 'name' => ['(\w+)', '255'] );
my %optional_params  = ();
my %validated_params = ();


# ensure all required params have been received, validate them
for my $param (keys %required_params) {
    
    unless ($q->param($param)) {
        error("Missing parameter $param\n");
    }
    
    my $regex = $required_params{$param};
    if ($q->param($param) =~ /$regex/) {
        $validated_params{$param}[0] = $q->param($param);
    } else {
        error("Invalid structure for parameter: $param");
    }
    
    unless (length($q->param($param)) <= $required_params{$param}[1]) 
+{
        error("Parameter: $param is too long\n");
    }        
        

}

# validate all optional params
for my $optional_param (keys %optional_params) {

    my $regex = $optional_params{$optional_param}[0];
    if ($q->param($optional_param) =~ /$regex/) {
        $validated_params{$optional_param} = $q->param($optional_param
+);
    } else {
        error("Invalid structure for parameter: $optional_param");
    }
    
    unless (length($q->param($optional_param)) <= $optional_params{$op
+tional_param}[1]) {
        error("Parameter: $optional_param is too long\n");
    }    
    
}


output_page();



sub error {

    # overkill, but allows flexibility in the future

    my $error = shift;
    
    print "Error: $error";
    
    exit();
    
}


sub output_page {
    
    # change to templating system for anything more than a few lines

print <<EOF;

<html>
<head>
<title>Thanks!</title>
</head>
<body>
<p>Thank you for your input.</p>
<p><a href="index.html">Return home</a></p>
</body>
</html>        
    
EOF

    exit();    


}

[download]

I'd really like to clean this code up but I'm not quite sure how. The one thing that comes to mind is to split each validation step up into subs. So I'd have a validate_length, validate_content, and exists sub of some kind. I'm not sure that would solve the problem though. I also don't know how I'd go about validating the optional and required parameters in the same loop.

I'd appreciate any suggestions you have :)

Edit by tye, added READMORE, moved from Meditations

In reply to Code Cleanup challenge! by Anonymous Monk

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":


  • 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! —
  • 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:
    & &amp;
    < &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.