[Israel.pm] Fwd: TelFOSS Soliciting aTalk aboutAutomated SoftwareTesting

Shlomi Fish shlomif at iglu.org.il
Wed Sep 22 05:50:48 PDT 2010


On Sunday 19 September 2010 19:43:46 Shlomi Fish wrote:
> On Sunday 19 September 2010 16:10:53 Levenglick Dov-RM07994 wrote:
> > https://docs.google.com/leaf?id=0B5HrAp2ZF5LqMDk3NmRlMTUtYmUxZS00ODg4LTh
> > hZGUtY2MwNzI4ZmYwMmY0&hl=en&authkey=CKfbgaYI
> 
> Your mailer broke the URL into two. Here is the unbroken URL:
> 
> https://docs.google.com/leaf?id=0B5HrAp2ZF5LqMDk3NmRlMTUtYmUxZS00ODg4LThhZG
> UtY2MwNzI4ZmYwMmY0&hl=en&authkey=CKfbgaYI
> 
> And here is a shortened URL:
> 
> http://xrl.us/bhzrfj
> 
> I initially believed it was a Google Document (at which point I wanted to
> ask if you had any embedded POD), but now that I've downloaded it, I see
> that it's a .zip file containing a few .pm files. I'll take a look at it
> later.


> 
> Just one note:
> 
> {{{
> use lib 'STP';
> }}}
> 
> This is not needed as "use STP::Array;" will already look under
> STP/Array.pm.
> 

OK, here are some other notes:

1. Your POD reads: "Each STP object" - please continue it.

2. 

{{{
require Exporter;

our (@ISA, @EXPORT, @EXPORT_OK, $VERSION, $DEBUG) ;

@ISA        = qw(Exporter);
}}}

This is better written as:

{{{
use parent 'Exporter';

our (@EXPORT, @EXPORT_OK, $VERSION, $DEBUG) ;
}}}

Or perhaps:

{{{
use base 'Exporter';

our (@EXPORT, @EXPORT_OK, $VERSION, $DEBUG) ;
}}}


3. 

[code]
@EXPORT     = qw(new add status ok passed find);
[/code]

Why are you exporting "new"? You shouldn't export the constructor. Also, 
aren't add() and friends methods which should be used as part of the class? If 
so, they shouldn't be exported, and you can get rid of inheriting from 
Exporter.pm.

4. The lines in your POD in STP.pm are too long. Please keep them at 80 
characters or 78 characters or so.

5. The synopsis for ->new reads:

    my $test = STP->new('name'=>'criteria_name', values=>[ {'value1'=>'val1' 
}, {'value2'=>'val2' } ]);

But it does not appear to match the constructor.

6.

{{{
my $class = ref($this) || $this;
}}}

Please don't do a $my_obj->new() :

http://www.stonehenge.com/merlyn/UnixReview/col52.html

7. The signature of add seems wrong - you don't appear to be accepting a hash 
as method arguments. Furthermore in the POD:

{{{
$test->add[\%criteria, %more_criteria];
}}}

This is invalid syntax.

8.

{{{ 
carp "You are creating an element that it already created!".
}}}

This is invalid English.

9. 

{{{
$this->[(TEST_LOC)]->{$hashref->{'name'}} = (ref $hashref->{'val'} eq 'ARRAY') 
?
            STP::Array->new($hashref) : STP::Scalar->new($hashref);
}}}

Maybe just require to pass STP::Array and STP::Scalar directly?

10. I also see you're using an array ref for the object with TEST_LOC as a 
slot name like this:

$this->[(TEST_LOC)]

Why not just use an accessor? See:

http://www.shlomifish.org/lecture/Perl/Newbies/lecture5/accessors/

10.

Some of the methods in STP.pm lack POD. Maybe try using POD::Coverage.

11. 

{{{
 map {return (STP_FAIL) if $_->status() != (STP_OK)} (values %{ $this-
>[(TEST_LOC)] });
}}}

Just use a foreach here. Or maybe use List::MoreUtils' any(), all(), notall() 
or none().

12. 

[code]
map {$ok = $_->analyze($verbose, $no_ret_on_err); return $ok if $ok != 
(STP_OK) and not $no_ret_on_err} (values %{ $this->[(TEST_LOC)] });
[/code]

Again, don't return from within a map.

13. 

[code]
sub find
{
    my ($this, $name) = @_;
    return (exists $this->[(TEST_LOC)]->{name}) ? $this->[(TEST_LOC)]->{name} 
: undef;
}
[/code]

find is a bad name for this method. Use lookup_$something or something.

-------------------------

I just covered STP.pm - I'll go over the other modules once my comments are 
fixed and a new version is uploaded.

Happy Sukkoth!

Regards,

	Shlomi Fish

-- 
-----------------------------------------------------------------
Shlomi Fish       http://www.shlomifish.org/
Why I Love Perl - http://shlom.in/joy-of-perl

<rindolf> She's a hot chick. But she smokes.
<go|dfish> She can smoke as long as she's smokin'.

Please reply to list if it's a mailing list post - http://shlom.in/reply .


More information about the Perl mailing list