Paul,
Here are my answers to your comments in post #3:
Quote:
1) "use lib" is used in the perl module but it isn't needed.
It is needed in my development process. Otherwise when checking for syntax with -c option I will get error. It can be commented out after release, though.
But causes NO problems.
Quote:
2) Data::Dumper debugging code is left in the code. That's not a major issue but it is loading a module that isn't needed. The reason for pointing this out too is that GT::Dumper is a better option for compatiblity sake (yes I know DAta::Dumper is standard but still).
Data::Dumper is used for debugging in development phase. I don't care about GT::Dumper (which module is not standard in Perl, but Data::Dumpler is). It can be commented out after release, tough. But since Data::Dumper is standard module in Perl releases will not cause any problems.
In overall, it causes NO problems.
Quote:
3) Excessive use of "my" at the top. my() is actually a sub-routine so there about 13 sub-routine calls when this could be reduced to one or two by declaring more than one variable at once.
You are right, that they could be declared in one step.
But usage of 13 "my" declarations does simply not matter in fact of the speed. The optimization you suggest is unnecessary, since in the full execution time these 13 "my"-s are executed in less than 1%. Optimization is unnecessary for such small speed gain.
Did you done profiling for DirName converter? I didn't, but I could say I'm 99% sure, that these 13 "my"s execution time, will be even not noted in the profiler result...
In overall the "my"s does NOT cause problems.
Quote:
4) "not" used where "!" would be more appropriate.
This does not mean any problems...
It's my programming style, and I like using not instead of ! for easier readibility.
Anyway it causes NO problems.
Quote:
5) Unnecessary escaping of single quotes inside qq||
Yeah, that's true. They are unnecessary. Earlier it was printed within '' quotes, then qq||; was added around the text later. Also I copied text from install.pm, where as you know '' quotes are used originally. I could correct all of them, but it does not cause any problems, so I don't bother about it currently.
In overall, it causes NO problems.
Quote:
6) Incorrect use of "ne" when comparing numerics. Should use "!="
Not incorrect. Both gives the same result, when comparing 2 integers.
When using 'eq' or 'ne' for number comparison, numbers are converted to strings automatically, then comparison is done as string.
Tough ne is likely slower a bit, but gives more readibility of the code (at least for me).
Finally since the speed decrease in under millisec, you don't have to bother about it. Also this plugin is not speed dependent, so speed does not matter.
In overall, it causes NO problems.
Quote:
7) Incorrect use of eq when comparing numerics. Should use "=="
Same as above, with the addition, that there are NO number comparisions using 'eq'!!!
You meant this code part?: $IN->param("step1") eq "1" .... also in html sourse: <INPUT TYPE="CHECKBOX" NAME="step1" VALUE="1">
All input parameters you get from the form are strings, so you statement is simply not true.
So it causes NO problems.
Quote:
8) Unnecessary use of parenthesis in regexes when no capturing is needed (this affects speed).
I checked the code, and yeah you are right, those parenthesis are unnecessary.
Those parentheses was used for grouping purpose, not for capturing reasons. I should use (?:) style groupings to avoid capturing or remove the parentheses from those regexps. Both would be saticfactory.
However yeah, the parenthesis capture affects the speed. But it does not mean so much speed decrease to bother with. Also DirName converter plugin has no speed demand at the moment.
I will place comments to the code to correct the captures, if there will be need for speed related optimizations in the plugin.
Quote:
9) Escpaing of forward slashes in regexes could be avoided by changing the substitution delimiter.
Yes, substitution delimiter could be changed. The code could be more readable.
However it causes NO problems in the code execution.
Quote:
10) Use of my() outside a sub-routine (thanks Yogi).
: "A my declares the listed variables to be local (lexically) to the enclosing block, file, or eval."
perdoc perlsub says: "You may declare my variables at the outermost scope of a file to hide any such identifiers from the world outside that file."
Having a my variable used in a module file is correct.
Paul, Yogi: you should try out this small example
:
Code:
my $text = "Test my variable in file\n";
package Foo::Bar;
use strict;
print "Content-Type: text/plain\n\n";
print $text;
my $text = "Test my variable in function\n";
print $text;
So your comment was NOT valid in CGI mode. Unfortunately under mod_perl the my() declaration usage on file level will not work if it's used under mod_perl. The mod_perl is the only reason to correct it, and avoid usage of my() declarations on file level.
Quote:
I think the plugin would fail under mod_perl.
I went & read some docs, and it seems you are right.
But mod_perl is only reason not to use my() declarations on file level.
Final conclusions:
Paul,
Thanks very much for taking your precious time to find bugs in my plugin!
In overall, I aggreed you that may be problems in 1.5 points. In point 8) and (in case the plugin is executed under mod_perl) in point 10).
The point 8) affects the speed a bit, but does not affect the correct work of the plugin.
I would say, the only one point, which has potential problems is the point 10), which affects the mod_perl usage.
Anyway, I appreciate your constructive criticizm!
Go ahead Paul, do hundreds of tests on my plugin, discover all the possible problems it has, and tell me all the problems!
I always appreciate if somebody does my work for free.
Best regards,
Webmaster33
Paid Support from Webmaster33. Expert in Perl programming & Gossamer Threads applications. (click here for prices)
Webmaster33's products (upd.2004.09.26) | Private message | Contact me | Was my post helpful? Donate my help...
