|Problems? Is your data what you think it is?|
Re: Could I get some feedback on my code please?by nobull (Friar)
|on Jan 13, 2007 at 11:40 UTC||Need Help??|
OK, I'm not looking at the macro scale, just the micro scale. Here are a few random observations.
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.
Is there a paricular reason you do this? If not, don't.
Is there a paricular reason you use our? If not use my.
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:
Do not use the special &-syntax to call subroutines unless you understand and require its special semantics.
Why do you split and rejoin the IP address?
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.
However, consider instead File::Slurp.
Why are you making the filehandle explict?