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?? |
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?
If I run your code through perltidy (as downloaded, with either configuration below)
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
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
Also consider renaming sendmail, for example, this is the description for one of them
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
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
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
In Section
Meditations
|
|