http://qs1969.pair.com?node_id=594524


in reply to Could I get some feedback on my code please?

OK, I'm not looking at the macro scale, just the micro scale. Here are a few random observations.
use LWP::Simple; use LWP::UserAgent; use HTTP::Request; use HTTP::Request::Common qw(GET); use HTTP::Response;

You never use HTTP::Request::Common::GET so why do you import it? LWP::UserAgent will load HTTP::Request and HTTP::Response for you. As far as I can see you never actually use LWP::Simple.


$|=1; #Disable buffering to allow instant output of text.

Is there a paricular reason you do this? If not, don't.


our $editorPassword = "**********"; our $language = "en"; # language code for wikipedia namespace our $enabled = "true"; # set to false to take offline/disable

Is there a paricular reason you use our? If not use my.


if ($enabled eq "true") {

It is my number one rule in programming: "use the most natural representation of things unless there is a reason to do otherwise".

If you want $enabled to be a boolean then use Perl's natural concept of "true" or "false" and simply say:

if ($enabled) {

&getInput;

Do not use the special &-syntax to call subroutines unless you understand and require its special semantics.

getInput();

my @gettheip = split(/\./,$ENV{'REMOTE_ADDR'}); my $remoteHost = "$gettheip[0].$gettheip[1].$gettheip[2].$gettheip[3]" +;

Why do you split and rejoin the IP address?


sub readEditTokens { &checkFileCanBeAccessed($editTokenFile, "READ"); open(TOKENFILE,"$editTokenFile") || &error("Cannot open edit token + file."); flock(TOKENFILE, 2) || &error("Cannot lock edit token file.") +; my $editTokens = <TOKENFILE>; flock(TOKENFILE, 8); close (TOKENFILE); return ($editTokens); }

The checkFileCanBeAccessed is ill-concieved. It adds complexity, introduces a race condition and delivers no beniefit.

You have useless quotes in "$editTokenFile".

You use the special legacy 2-arg open() syntax in which perl parses the second argument into two parts: mode and filename. It is simpler and clearer to use the 3-arg format.

You are using the global file handle TOKENFILE. This habit could cause you troble in future. You could local(*TOKENFILE) to make your changes to the global handle temporary but better to simply not use a global handle.

You don't bother to check for errors on close(). This is not too bad but if you localised the filehandle you wouldn't need to close it explicitly (unless to check for errors).

Your indentation is confusing.

It is more conventinal to use or not || for flow control.

Error messages should contain the error.

You are taking an exclusive lock when you are just reading.

There's no reason to unlock a handle you are about to close anyhow.

sub readEditTokens { open(my $TOKENFILE, '<', $editTokenFile) or error("Cannot open edit token file: $!"); flock($TOKENFILE, 1) or error("Cannot lock edit token file: $!"); my $editTokens = <$TOKENFILE>; return ($editTokens); }

However, consider instead File::Slurp.


print STDOUT "Content-type: text/html\n\n";

Why are you making the filehandle explict?