Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl: the Markov chain saw
 
PerlMonks  

Re: Could I get some feedback on my code please?

by nobull (Friar)
on Jan 13, 2007 at 11:40 UTC ( [id://594524]=note: print w/replies, xml ) Need Help??


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?

Replies are listed 'Best First'.
Re^2: Could I get some feedback on my code please?
by PockMonk (Beadle) on Jan 13, 2007 at 12:13 UTC

    Wow, lots of stuff to look at there, thanks. I have a few questions though. you say:

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

    My copy of the Programming Perl book makes no mention of this, should I never use the "&" in calling subroutines??? This is the first I've heard of this! Can you eplain please?

    "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. "

    Can you point me to a page on this or explain this further please?

      Calling a sub with &sub will do many things, but most interstingly, it will not set @_ inside the sub, so you will inherit the @_ from the outer context, the & also means that prototypes are ignored for that call.

      If you're not sure what those are, it's safer to not tamper with them, and just call subs with sub().

      Somewhere it says in Learning Perl, "you don't tell someone to 'do wash the dishes' when you can just say 'wash the dishes'" this is a terrible paraphrase, but i think i left the book at work ...

      @_=qw; ask f00li5h to appear and remain for a moment of pretend better than a lifetime;;s;;@_[map hex,split'',B204316D8C2A4516DE];;y/05/os/&print;
      Look at perlsub: &subname(); will disable prototype-checking for that call (now prototypes aren't used much in perl, for good reasons, but if they're there, you'll probably don't want to skip the checks).

      Also, if you forget the parenthesis and call the sub as &subname; you will forward your current @_ array to that call. In other words, you will pass on your current arguments - not call it without arguments.

      Instead of doing that, you should probably always write subname( args ) (i.e. with parenthesis and without the ampersand). It's the only way to call a subroutine that doesn't have any hidden snags. Generally, you'll only use the & sygil if you want to take references to named subroutines or for goto &subroutine which is only useful in very specific cases.

      using "&" is not needed in most parts of your code, but is still used for some things.
      From what I have read, tells to save the use of "&" for calling subroutines to equale other stuff like
      $SIG{__DIE__} = \&fatal_error;
      and other uses.
      Perl 5 still supportes "&" but the preferd format is like "a_sub();" the "&" is considered as clutter.

      A Link to Chapter 9 of Perl Best Practices page 176 explains this and I suggest to reading the hole Chapter and/or buy the book.

      Good Luck

      About the file handle thing: in Perl 5 a file handle can be stored as a reference in a scalar variable. As of Perl 5.6 (I think), the builtin open function can initialize this variable for you:
      open my $tokenfile, '<', $filename or die "Couldn't open file: $!";
      Now $tokenfile is your filehandle and you don't have to worry about it being global or whatever. Have a read through perlopentut - it talks more about this.

Log In?
Username:
Password:

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

How do I use this?Last hourOther CB clients
Other Users?
Others admiring the Monastery: (8)
As of 2024-03-28 15:06 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found