in reply to RFC: beginner level script improvement
version control before making any changes, so you can see how the code evolves (and you can undo any changes)
Just curious, what is your monitor/editor size, what do you see?
I have 1024x768 with font:DejaVu Sans Mono,size:11, and I see , I see 103 chars/columns by 33 lines
If I go to font-size 10 I can see your 120 char
If I run your code through perltidy (as downloaded, with either configuration below)
## perltidy -olq -csc -csci=10 -cscl="sub : BEGIN END" -otr -opr -ce +-nibc -i=4 -pt=0 "-nsak=*" ## perltidy -olq -csc -otr -opr -ce -nibc -i=4 -pt=0 "-nsak=*"
The line count increases from ~1401 to ~1716, ~305 lines added for readability :)
And perltidy warns about having two functions named sendmail -- one from eod_functions and one from your main script
And perltidy also helps you keep consistent indentation level( you have some inconsistent ones)
Another aide to readability is increasing skimmability ( skimmable code is the idea ), that is replace long if/else blocks with functions, for example replace
my ($NL,$SKIP) = ($STATIC{NL},$STATIC{SKIP}); if ($STATIC{CHAR_NL} eq "NL"){ $NL = "\n"; }elsif($STATIC{CHAR_NL} eq "CR"){ $NL = "\r"; }elsif($STATIC{CHAR_NL} eq "CRLF"){ $NL = "\r\l"; }elsif($STATIC{CHAR_NL} eq "HEXNL"){ ##still needs testing $NL = ""; } if ($STATIC{CMD_SKIP} eq "SPACE"){ $SKIP = "\032"; }elsif($STATIC{CMD_SKIP} eq "NL"){ $SKIP = $NL; }else{ $SKIP = "$STATIC{CMD_SKIP}$NL"; }
my( $NL, $SKIP ) = optionize_nl_skip( \%STATIC );
I put in optionize cause its close but not quite a good name to describe what the function does, beside return nl and skip
Why write it this way? Its easier to test , like this
use strict; use warnings; use Test::More qw/ no_plan /;; { my( $NL, $SKIP ) = optionize_nl_skip( { qw/ CHAR_NL NL CMD_SKIP SP +ACE / }); is($NL, "\n" ); is($SKIP , "\33" ); ## Test::More printable bug } sub optionize_nl_skip{ my( $static ) = @_; my ($NL,$SKIP) = ( $static->{NL}, $static->{SKIP} ); if ($static->{CHAR_NL} eq "NL"){ $NL = "\n"; }elsif($static->{CHAR_NL} eq "CR"){ $NL = "\r"; }elsif($static->{CHAR_NL} eq "CRLF"){ $NL = "\r\l"; }elsif($static->{CHAR_NL} eq "HEXNL"){ ##still needs testing $NL = ""; } if ($static->{CMD_SKIP} eq "SPACE"){ $SKIP = "\032"; }elsif($static->{CMD_SKIP} eq "NL"){ $SKIP = $NL; }else{ $SKIP = "$static->{CMD_SKIP}$NL"; } return( $NL, $SKIP ); }
Also consider renaming sendmail, for example, this is the description for one of them
## SUB SENDMAIL ### DOES: generates and sends e-mail to all recipie +nts with attached csv report ##################
Maybe call it mail_report() or send_report() or report_result( \%REPORT,\%CONFIG,$RESULT )
Yeah, report_result, cause it explains action and object (send_result_report)
Other things that need comments (explanation) are regular expressions, because this
doesn't explain what $TEMP is, or why it needs ...numberregex... removed .... but the whole block should be a function call, something like$TEMP[0] =~ s/^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[ +0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)://;
{ my( $commands, $matches ) ; ( $_, $commands, $matches ) = sanitize_and_then_some_c +ommands_matches( $_, \@METACHARS,\@TRANSLATIONS ); $DATA{$IP}->{COMMANDS} = $commands; $DATA{$IP}->{MATCHES} = $matches; }
Other minor things , style related, is this unless ( $SHELL == '2' || $SHELL == '1' ) it will work, warnings won't complain, but if you're using numeric comparison operator == you might as well save yourself some typing of '' :) see perlnumber
More on style is if ($#ARGV < 0 ) { &help($0); exit 0;} is perlishly written as if( ! @ARGV ){ help($0); exit 1; }
or @ARGV or die help($0);
Speaking of exit, if you put your main program into a subroutine Main you can write Main( @ARGV ); exit( 0 ); More on this idea (and modulinos ) see Re: No such file or directory error/No such file or directory error, see template at (tye)Re: Stupid question (and see one discussion of that template at Re^2: RFC: Creating unicursal stars
Other minor issues are 2 argument open instead of 3 argument open, see open
Sometimes you use &function() but sometimes you use function() -- 99% of the time you don't need the & at all, so think about your reason for using it, then use the same style everywhere (good idea is to prefer less typing :)
speaking of function names and documentation, sub taint is poorly named, as taint has a specific meaning in perl and it isn't escape_ctrl, which is a better name and more documenting
speaking of documentation, PARSETEMPDB doesn't describe the database format , maybe you have an example database somewhere which describes the format, so this should be mentioned
another style thing is how you're UPPERCASENAMING lots and lots and lots and lots and lots of variables, there are various style recommendations about that, one being perlstyle
$ALL_CAPS_HERE constants only (beware clashes with perl vars!) $Some_Caps_Here package-wide global/static $no_caps_here function scope my() or local() variables
once you make some of these changes it becomes easier to see what your program is doing, how it works, and what other changes might be recommended
version control before making any changes, so you can see how the code evolves (and you can undo any changes)
start a new directory for each block/change :) poor-mans version control
|
---|
Replies are listed 'Best First'. | |
---|---|
Re^2: RFC: beginner level script improvement (version control)
by georgecarlin (Acolyte) on Sep 20, 2013 at 11:43 UTC | |
by Anonymous Monk on Sep 20, 2013 at 12:48 UTC |