in reply to Need help with Validation script

Seriously, it took me about 5 minutes to understand your lengthy code, so I really wonder - what have you done so far to find out where your code does go wrong? Have you traced it in the debugger? Have you sprinkled diagnostic print or warn statements?

As a hint, Perl does not consider two strings identical if they differ in case. For example the following prints not equal:

my $col_name = 'P_DLRUOM';
my $entered_value = 'p_dlruom';

sub check_colname {
    if ($col_name eq $entered_value) {
        return 'equal';
    };
    return 'not equal';
};

[download]

If this hint is not yet enough information for you, compare the next code with the code you have in validate_fields:


sub validate_fields {
    my ($colvalue,$colname) = @_;

    if ($colname eq 'P_DLRUOM') {
        warn "Checking '$colvalue' for validity in 'P_DLRUOM':";
        return validate_p_dlruom( $updateval );
    } elsif ($colname eq 'FOO') {
        warn "Checking '$colvalue' for validity in 'FOO':";
        return validate_foo( $updateval );
    } else {
        warn "Unknown column name '$colname' passed.";
        return;
    };
};

[download]

Coincidentially, your whole data setup is horrible - you should store the whole information about your table in a data structure and then use that data structure to run your program instead of hardcoding all the column names and possible values. Perl has a very good data structure for such things, the hash. It maps names to other things, for example subroutines:

# Map the column name to the validator code:
my %validator = (
    P_12MONTHDEMANDQTY => \&validate_number,
    ...
    P_DLRUOM => \&validate_p_dlruom,
    ...
);

sub validate_fields
{
    my ($updateval, $colname) = @_;
    if (! exists $validator{ $colname }) {
        warn "'$colname' is not a valid column name.";
    };
    my $code = $validator{ $colname };
    if (! $code->($updateval, $colname)) {
        warn "'$updateval' is not a valid value for '$colname'.";
    } else {
        warn "'$updateval' is a valid value for '$colname'.";
        return 1
    };
}

[download]

Far fewer opportunities to forget a value or to return the wrong value with this. You might want to do a search on dispatch tables here to learn more.

Replies are listed 'Best First'.
Re^2: Need help with Validation script
by ssmith001 (Initiate) on Jul 16, 2007 at 19:16 UTC
    Thanks for your input. I'm still trying to get my hands around this concept of dispatch tables by construnction a small example here below: 
    #!/usr/bin/perl -w


my @p_dlruom = ("EACH","FEET","FT","IN","UNIT");

my %validator = (
    P_12MONTHDEMANDQTY => \&validate_number,
    P_DLRUOM => \&validate_p_dlruom
);


validate_fields(EACH, P_DLRUOM);



sub validate_fields
{
    my ($updateval, $colname) = @_;

    #print "$updateval $colname\n";

    if (! exists $validator{ $colname }) {
        print "'$colname' is not a valid column name.";
    };

    my $code = $validator{ $colname };

    if (! $code->($updateval, $colname)) {
        print "'$updateval' is not a valid value for '$colname'.";
    } else {
        print "'$updateval' is a valid value for '$colname'.";
        return 1
    };
}

sub validate_p_dlruom
{
 my $val = shift;

 my @match = grep (/$val/i, @p_dlruom);

 print "The value of $val you have chosen for P_DLRUOM is outside of t
+he allowable range.\n" if !(@match);

}

    [download]
[reply]
[d/l]

      I'm not sure exactly what feed back you are looking for on this code, but I have a couple of comments:

      • Be careful passing around barewords. Use strict and warnings to catch this. In your code, I'm specifically referring to this line: 
        validate_fields(EACH, P_DLRUOM);

        [download]
        Which will error under strictures and who knows what it will do otherwise. Instead, make sure you quote the words: 
        validate_fields('EACH', 'P_DLRUOM');

        [download]
        or: 
        validate_fields( qw( EACH P_DLRUOM ) );

        [download]
      • You are checking for the return value of your coderef, but the coderef you call doesn't actually explicitly return anything. In its current state, it will always return true, or rather, it will return the result of the print statement, as it is the last thing that happens in the subroutine
      • This doesn't make a big difference, but be careful about how you pass arguments. validate_p_dlruom only looks at the first argument, but you pass two. Generally, I like to do this: 
        foo( { arg1 => 'val1', arg2 => 'val2' } );

sub foo{
    my $arg_ref = shift;

    # do some stuff with $arg_ref->{arg1} and
    # $arg_ref->{arg_2}

...

    return;
}

        [download]
        Then you have a better idea of what you are passing around and you don't have to rely on the order in which they are passed. 
      perl -e 'split//,q{john hurl, pest caretaker}and(map{print @_[$_]}(joi
+n(q{},map{sprintf(qq{%010u},$_)}(2**2*307*4993,5*101*641*5261,7*59*79
+*36997,13*17*71*45131,3**2*67*89*167*181))=~/\d{2}/g));'

      [download]
[reply]
[d/l]
[select]
        I guess what I was looking for was some help with a simple routine to check a single column P_DLRUOM to see if it's value is one of ("EACH","FEET","FT","IN","UNIT"). If the user inputted value for P_DLRUOM = "EACH", it's valid and then I want to run off an build the SQL statement to do the update. I can take care of that part later. Since there will be numerous column to check eventually, it sounds like to the best way would be to use as hash as suggested above.
[reply]

In Section Seekers of Perl Wisdom