Please critize my scripts and suggest better ways to
accomplish the same task.
(I know it's a good practice to put use strict in Perl
script but I'm struggling to make my scripts work with
use strict.The script below work fine if I comment out
use strict.)
sybase@titania /home/sybase/scripts/pl =>
test_load_lcmr.pl 275 2005 06
Global symbol "$isql" requires explicit package name
at ././test_load_lcmr.pl line 54.
Global symbol "$loader" requires explicit package name
at ././test_load_lcmr.pl line 66.
Global symbol "$loaderpswd" requires explicit package
name at ././test_load_lcmr.pl line 66.
Global symbol "$isql" requires explicit package name
at ././test_load_lcmr.pl line 78.
Execution of ././test_load_lcmr.pl aborted due to
compilation errors.
____________________________________________________
Yahoo! Sports
Rekindle the Rivalries. Sign up for Fantasy Football
http://football.fantasysports.yahoo.com
Full Version: Critcize my scripts.
loan tran wrote:
| QUOTE |
| Please critize my scripts and suggest better ways to accomplish the same task. |
You asked for it.
| QUOTE |
| (I know it's a good practice to put use strict in Perl script but I'm struggling to make my scripts work with use strict.The script below work fine if I comment out use strict.) |
Turn on use strict.
Turn on use strict.
Turn on use warnings.
Turn on use strict.
| QUOTE |
| sybase@titania /home/sybase/scripts/pl = test_load_lcmr.pl 275 2005 06 Global symbol "$isql" requires explicit package name at ././test_load_lcmr.pl line 54. Global symbol "$loader" requires explicit package name at ././test_load_lcmr.pl line 66. Global symbol "$loaderpswd" requires explicit package name at ././test_load_lcmr.pl line 66. Global symbol "$isql" requires explicit package name at ././test_load_lcmr.pl line 78. Execution of ././test_load_lcmr.pl aborted due to compilation errors. |
You are getting these errors because you have not declared all of your
variables. A great place to start:
http://perl.plover.com/FAQs/Namespaces.html
| QUOTE |
sybase@titania /home/sybase/scripts/pl => test_load_lcmr.pl 275 2005 06 Global symbol "$isql" requires explicit package name at ././test_load_lcmr.pl line 54. Global symbol "$loader" requires explicit package name at ././test_load_lcmr.pl line 66. Global symbol "$loaderpswd" requires explicit package name at ././test_load_lcmr.pl line 66. Global symbol "$isql" requires explicit package name at ././test_load_lcmr.pl line 78. Execution of ././test_load_lcmr.pl aborted due to compilation errors. #----------------- script name: test_load_lcmr.pl -------------------------- #!/usr/bin/perl # Purpose: bcp lcmrldr ftp file provided by CPMC into lcmrdb..ldr_stage and exec sp to load lcmr #use strict; use warnings; require "/home/sybase/scripts/pl/global.pl"; |
You might consider making the above dynamic or relative. And 'use' is
often favored to 'require' these days.
perldoc -f use
perldoc lib
perldoc FindBin
| QUOTE |
| our $recipients = '[Email Removed]'; our $dockdir ="/ftpsite/as400/glmgr"; #our $isql = "/opt/finance/sybase/OCS-12_0/bin/isql -U$loader -P$loaderpswd -w133 "; #now get from global.pl our $bcp = '/opt/finance/sybase/OCS-12_0/bin/bcp'; our $company = <$ARGV[0]>; our $year = <$ARGV[1]>; our $period = <$ARGV[2]>; |
You might try using Getopt::Long for option processing. I have a sample
template available at:
http://danconia.org/cgi-bin/request?handle...t_long_template
| QUOTE |
| our $server ='THEBRAIN'; # for testing our $debug =0; our $load_file = 'cpmc_lcmr_'.$year.'_'.$period.'.'.'csv'; our $log_file = '/home/sybase/scripts/logs/load_lcmr.pl.log'; qx (cp /dev/null $log_file ); |
You shouldn't shell out for the above process which appears to just
empty a file. You can use Perl builtins for that, and should. When
copying a file you should consider using File::Copy.
| QUOTE |
| #print "nserver = $server; company = $company; Year = $year; period = $period n"; qx(echo "server = $server; company = $company; Year = $year; period = $period n" >>$log_file); |
There is no reason to shell out to echo, print should work fine. Why the
commented out line above? If you are trying to write to the log file,
you should open it for printing.
perldoc perlopentut
perldoc -f open
perldoc -f print
perldoc -f close
| QUOTE |
| ##### Main Part ############## if($#ARGV != 2 or $company !~ /^d{3}$/ or $year !~ /^d{4}$/ or $period !~ /^d{2}$/){ print "nIncorrect arguments. See usage below:n"; &Usage; |
Using & is the older method, the above is better written:
usage();
But see my template on a nice way to handle option processing, usage
statements, and documentation all rolled into one.
| QUOTE |
| } else{ unless(-f "$dockdir/$load_file"){die qq(Source file "$dockdir/$load_file" not found. Script aborted.n); } &deleteBeforeLoad($company,$year,$period); |
Again, no '&'.
| QUOTE |
| &bcp_in; |
Same, the above should take its arguments and optionally return
value(s). You are using globals in your subroutines which is evil,
strict will help, as will moving your subs into a library.
| QUOTE |
| if ($debug ==0){&run_lcm2_post($company,$year,$period)} #end if; ¬ify_mail; } #end else ###### Sub Routines ############### sub Usage{ print "n********* USAGE ***********n"; print "n$0 Company Year Periodn"; print "Company: 3 digits required (275)n"; print "Year : 4 digits required (2005)n"; print "Period : 2 digits required (06)n"; print "n**************************n"; exit; } #end sub Usage sub deleteBeforeLoad{ print "---Delete staging table before bcp in n"; qx($isql -S$server<<EOF>> $log_file |
You might consider using the DBI module and its drivers depending on
what database this is.
| QUOTE |
| select getdate() go exec lcmrDB..delete_LDR_Stage $_[0],$_[1],$_[2] go EOF ); &check_error; } #end sub delete sub bcp_in{ print "---Start bcp in n"; qx($bcp lcmrDB..LDR_Stage in $dockdir/$load_file -c -U$loader -S$server -P$loaderpswd -t',' >>$log_file ); |
Again, dispense with the shelling out and move any commands that can be
handled in Perl into the native code. Especially simple things like
'wc', 'cut', 'echo', etc.
[snip]
The rest of the script follows the same issues as above.
Good luck,
http://danconia.org
loan tran wrote:
| QUOTE |
| Please critize my scripts and suggest better ways to accomplish the same task. (I know it's a good practice to put use strict in Perl script but I'm struggling to make my scripts work with use strict.The script below work fine if I comment out use strict.) |
They look more like shell scripts than Perl programs.
Don't use qx() for everything, use Perl's built-in operators/functions or if
you have to, use system().
perldoc perlop
perldoc perlfunc
Don't use an ampersand in front of function names.
perldoc perlsub
John
--
use Perl;
program
fulfillment
Thank you John and Wiggins for your time and
feedbacks.
Loan
--- "John W. Krahn" <[Email Removed]> wrote:
feedbacks.
Loan
--- "John W. Krahn" <[Email Removed]> wrote:
| QUOTE |
| loan tran wrote: Please critize my scripts and suggest better ways to accomplish the same task. (I know it's a good practice to put use strict in Perl script but I'm struggling to make my scripts work with use strict.The script below work fine if I comment out use strict.) They look more like shell scripts than Perl programs. Don't use qx() for everything, use Perl's built-in operators/functions or if you have to, use system(). perldoc perlop perldoc perlfunc Don't use an ampersand in front of function names. perldoc perlsub John -- use Perl; program fulfillment -- To unsubscribe, e-mail: [Email Removed] For additional commands, e-mail: [Email Removed] <http://learn.perl.org/ <http://learn.perl.org/first-response |
____________________________________________________
Yahoo! Sports
Rekindle the Rivalries. Sign up for Fantasy Football
http://football.fantasysports.yahoo.com
This is a "lo-fi" version of our main content. To view the full version with more information, formatting and images, please click here.