Re: [perl-win32-gui-users] Code Review?

[Johan L. <jo...@ba...>]

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                     jo...@ba...
                  http://www.bahnhof.se/~johanl/
If the only tool you have is a hammer,
everything tends to look
like a nail