Beefy Boxes and Bandwidth Generously Provided by pair Networks
No such thing as a small change
 
PerlMonks  

Re: RFC: beginner level script improvement (version control)

by Anonymous Monk
on Sep 20, 2013 at 00:37 UTC ( [id://1054923]=note: print w/replies, xml ) Need Help??


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"; }
with
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

$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]?)://;
doesn't explain what $TEMP is, or why it needs ...numberregex... removed .... but the whole block should be a function call, something like
{ 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
    @all Thank you all for taking the time and secondly for the many helpful hints and pieces of advice. As most of you will have guessed I'm not a programmer but a network engineer in an ISP surrounding so any references to other languages will be lost on me. I only know a little perl and even less bash scripting =). I have set up dir based version control as suggested, svn may not be installed on the server. I will change open, VAR and function naming and formatting as suggested. I will also get rid of redundant #-style formatting as pointed out. And The print here reference is very helpful too. Regarding the function calling I chose to call module-based functions without leading & and vice versa. I will change that too though. Once I have implemented all of the above I will try and work out a more sub'ed style and present the new and hopefully improved version. font (FixedSys) is what vim.common defaulted to in size 9 @1600x900. Format works in vi on Solaris too with the same display settings, also in FullHD on ubuntu in vim.common.

      font (FixedSys) is what vim.common defaulted to in size 9 @1600x900

      ah gvim, I see :) gvimportable also does that on win32, I get L39/W121 (with line numbering of course) and readability doesn't suffer

      Having played with this a bit, I see whats readable about fixedsys(9), its more bold :) the boldness really seems to contribute to readability

      L39/W121 DejaVu Sans Mono Bold,size:10 does a nice impression of fixedsys(9) but it isn't quite as bold(thick)

      even deja..11 bolds isn't quite as bold as fixedsys(9)

      Thanks

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: note [id://1054923]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others sharing their wisdom with the Monastery: (4)
As of 2024-03-28 21:18 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found