On Thu, Mar 08, 2007 at 07:15:49AM +0100, Jonas B. Nielsen wrote:
> Well all are included, version 0.01 is available on CPAN.
I've had a look, and I have the following comments to make:
* You seem to have a mixture of tabs and spaces for indentation, and the
tabs seem to be spaced at 4 spaces. Please be consistent - tabs only
or spaces only. My personal suggestion is that we've no need to use a
real \t when we can put 8 spaces in instead - we're not stuck on
4kbyte microcomputers any more. If you feel you must use real \t
characters, make them equal to 8 spaces.
(vim's :retab command is likely to be useful in fixing up this one)
* You've put all the POD at the end of the file. I usually find that if
I put the POD for each function just above the function code itself,
I'm much more likely to keep it updated when I change the code. Makes
it easier to find too, I think...
* Line 25:
my $alarm = 2; #default alarm
That lexical variable is not visible from outside the module - your
documented suggestion of
$test::Timer::alarm = 6;
is not going to work. Instead use a package variable; viz.:
our $alarm = 2;
* Line 16:
use lib qw(../../lib);
I expect that's for testing purposes, is it? You don't need to do
that as the test framework will ensure the correct paths appear in
@INC.
* The code bodies of time_ok() and time_nok() seem almost identical.
Perhaps you could find a way to refactor the code around to have a
common implementation?
* A test warning:
t/time_nok........ok 1/3Name "Test::Timer::alert" used only once:
possible typo at t/time_nok.t line 10.
But apart from those, it looks to be coming along the right way :)
Certainly, the look of the interface in the documentation seems just
what I had imagined.
--
Paul "LeoNerd" Evans
[EMAIL PROTECTED]
ICQ# 4135350 | Registered Linux# 179460
http://www.leonerd.org.uk/
