strict and declare subroutine argument number and order

nlafferty has asked for the wisdom of the Perl Monks concerning the following question:

I am hoping that one of you can help me out with this. In a recent project I have, I was told that my script need to run under strict and that the Subroutine declare it number and order of arguments. I understand that both are a really good idea for debugging and generally good coding practice, but i'm just starting to move from...dare i say, all global varaiables(well that's how i started learning). But I want implement the use of the proper scoping in the following script. It's was suggested that i make a package. Can any one bring light to my code? The biggest problem is with the SQL subroutines being access from the other script.

Here is my lib of subs and stuff

#####################################################
##common.sub                                                ##
## This file contains subroutines and 
## variables                                  
##
## that are used in multiple ##scripts.                               
+                   ##
## Configure what is ##needed.                                        
+                  ##
#####################################################


## Authentication Variables

#my ($user, $admin);
$user = $ENV{"REMOTE_USER"};
$admin = "admin";       # Set to the Administrators username

##########################################
## Authentication Error
##########################################
sub print_error{
  print <<HTML;
<HTML> <HEAD>
<TITLE>Error</TITLE>
</HEAD>
<BODY>
<CENTER><B><FONT size=3><P> Authentication Failed. Please Restart your
+ Browser.</FONT></B></CENTER><HR>
</P>
</BODY></HTML>
HTML
} # End of print_results subroutine
##########################################
## Create a connection to the database. ##
##########################################
sub Create_DB_Connection{
use DBI;
use DBD::Pg;
my ($DSN, $db_user, $pw);

$DSN  = "DBI:Pg:dbname=yaya";
$db_user = "yaya";
$pw   = "yayapwd";

$dbh  = DBI->connect($DSN,$db_user,$pw, { RaiseError => 1 })
|| die "Cannot connect: $DBI::errstr\n" unless $dbh;
return;
} # End of Create_DB_Connection subroutine.

##########################################

##########################################
## Executes the SQL command and then    ##
## returns to the calling area of the   ##
## program.                             ##
##########################################
sub Do_SQL{
    eval{
    #my $sth;
    $sth = $dbh->prepare($SQL);
  }; # End of eval

  # Check for errors.
  if($@){
    $dbh->disconnect;
    print "Content-type: text/html\n\n";
    print "An ERROR occurred! $@\n<P>";
    exit;
  } else {
    $sth->execute;
  } # End of if..else
  return ($sth);
} # End of Do_SQL subroutine
#################################################################

####################################
### Filter - Gets rid of         ###
### characters that screw up the ###
### program.                     ###
####################################
sub filter{
  $_[0]=~s/\'/\\\'/g;
  return $_[0];
} # End of filter subroutine
#################################################################

1;  # Required or won't work!
[download]

Here is the main script- i removed some subs because the are irrelivant.

#!/usr/bin/perl

use CGI qw/:standard/;
require "common.sub";

#my ($date, $time, $username);

$username = "$user";
&in_time_date;

#get time/date
sub in_time_date{
#my ($min, $hour, $day, $month, $year, $calc_year, $calc_month);
($min, $hour, $day, $month, $year) = (localtime)[1,2,3,4,5];
$calc_year        = $year+1900;
$calc_month     = $month+1;
$date = "$calc_year-$calc_month-$day";
$time = "$hour:$min";
}

print header;

if($username eq "$admin"){
&Create_DB_Connection;
&admin_get_users_in;
&print_admin_data;
}
elsif ($username eq ""){
&print_error;
}else{
    &Create_DB_Connection;
    &check_for_in;
    }
$dbh->disconnect;

################ BEGIN GET USERS CLOCKED IN SUBROUTINE - ADMIN
sub admin_get_users_in{
  $SQL="SELECT oid,username,start_stamp FROM timeclock WHERE total_hou
+rs = NULL AND total_minutes = NULL AND end_stamp = NULL ORDER BY user
+name";
    &Do_SQL;
}
################ END GET USERS CLOCKED IN SUBROUTINE

################ CHECK IF CLOCKED IN SUBROUTINE - NORMAL
sub check_for_in{
  $SQL="SELECT oid,* FROM timeclock WHERE end_stamp = NULL AND total_h
+ours = NULL AND total_minutes = NULL";
    &Do_SQL;

     @clocked_in = $sth->fetchrow();
          unless ($clocked_in[1] eq "$username"){
                &clock_in_form;
          }else{
              $SQL="SELECT oid,* FROM timeclock WHERE username = '$use
+rname' AND oid = $clocked_in[0]";
              &Do_SQL;

             while(@out_oid = $sth->fetchrow()){

            @date_array01 = split(/ /,$out_oid[2]);
            $print_date = $date_array01[0];
            $in_time_sec = $date_array01[1];
            @in_time_array = split(/:/,$in_time_sec);

             $in_time = join(":",$in_time_array[0],$in_time_array[1]);
             $hidden_oid = $out_oid[0];
             &clock_out_form;
     }
           }

}
################ END CHECK FOR IN SUBROUTINE
[download]

Comment on strict and declare subroutine argument number and order Select or Download Code

Replies are listed 'Best First'.
Re: strict and declare subroutine argument number and order by Zaxo (Archbishop) on Nov 17, 2001 at 01:33 UTC
A few points: '`use`' instead of '`require`'. Then the inclusion is done at compile time and the subs can be found early by the compiler. `use warnings;` That will tell you what's not to like. In older Perl, make that '-w' on the shebang line. `use strict;` If you don't actually declare that, it can't tell you what's wrong. You have globals in the library, and making them lexical does not improve matters. The place to deal with an instance variable like $user is in the main script, where it should be lexical. If you truly have global parameters, consider `use constant ADMIN => 'admin';` At the top of your library, just say `package Site::common;` and save it as 'Site/common.pm'. No OO needed it can just be a bag of subroutines. I recommend perlmod, perlsec, and perlvar. Update: crazyinsomniac reminds me that warnings may also be turned on by `$^W=1;`. After Compline, Zaxo	[reply] [d/l] [select]

Replies are listed 'Best First'.

Re: strict and declare subroutine argument number and order
by Zaxo (Archbishop) on Nov 17, 2001 at 01:33 UTC

A few points:

'use' instead of 'require'. Then the inclusion is done at compile time and the subs can be found early by the compiler.
use warnings; That will tell you what's not to like. In older Perl, make that '-w' on the shebang line.
use strict; If you don't actually declare that, it can't tell you what's wrong.
You have globals in the library, and making them lexical does not improve matters. The place to deal with an instance variable like $user is in the main script, where it should be lexical. If you truly have global parameters, consider use constant ADMIN => 'admin';
At the top of your library, just say package Site::common; and save it as 'Site/common.pm'. No OO needed it can just be a bag of subroutines.

perlmod

perlsec

perlvar

Update: crazyinsomniac reminds me that warnings may also be turned on by $^W=1;.

After Compline,
Zaxo

[reply]
[d/l]
[select]