Morbus wrote:
I'm looking for a code review of the below - mainly in the realms of
Windows correctness, and what I'm missing, and so forth. The code below is
a drop-able library into my AmphetaDesk
(http://www.disobey.com/amphetadesk/), but could work for some other
programs as well.
Basically I would say it looks fine. I'm a bit curious about that
double-event-thingy though, sounds a little odd.
Ok, the code review, here goes :
* The $SETTINGS global hash ref isn't documented anywhere and could
probably be passed to the various subs instead. Globals config variables
has always bitten me sooner or later in the past :/
* In open_url:
use Win32::API;
my $ShellExecute = new Win32::API("shell32", "ShellExecuteA",
['N','P', 'P', 'P', 'P', 'I'], 'N');
I would create the API object once at compile time (outside the sub) and
check for success (or die("something")). That way you will have a safe
state when the application starts--no surprises in the middle of everything
you have to recover from.
* What does
$logbox->SendMessage(0x301,0,0);
mean? You won't remember in two weeks either. Encapsulate in a generic and
well-named sub that gives away what it is doing. Beats a comment any day
(not that you shouldn't document the sub :)
* sub _OpenWindow_Click {
What happens if open_url takes a long time and the user double clicks? Set
the "$already_called = 1;" immediately after checking it, making it as
atomic as possible.
As a personaly coding style, when I have a global variable
($already_called) which is in effect only a static variable (only used in
that sub), I declare it in close proximity to the sub, i.e. just before the
sub, and make sure it gets initialized with something useful. Readability
and maintainability reasons.
[oops, saw that you used it in many subs now :) the point is still valid
in that context though]
/J
------ ---- --- -- -- -- - - - - -
Johan Lindström Boss Casinos
Sourcerer [EMAIL PROTECTED]
http://www.bahnhof.se/~johanl/
If the only tool you have is a hammer,
everything tends to look
like a nail
