Some questions on CDBI code

[prev] [thread] [next] [Date index for 2005/07/05]

Hi Y'all,

    I'm helping with the Phalanx project, specifically the attempt to 
write coverage tests for Class::DBI so that Devel::Cover reports 100% 
coverage of all statements, branches, conditions and subroutines for all 
the components (and dependencies) of CDBI - you can see how were going 
at http://phalanx.kwiki.org/index.cgi?ClassDBIHoplites

So in doing all this, some areas of the code puzzle me, so I'd thought 
I'd ask before suggesting changes.

In Class::DBI::ColumnGrouper::primary(), the sub tries to detect if 
you've called it in a non-array context and you have a multicolumn 
primary key. (code is if (!wantarray && @cols > 1) ).
If this is true, it calls Carp::confess, which of course dies. It then 
tries to return the first of the primary key names - but it cant - cause 
its died. And hence the line "return $cols[0];" is reported by 
Devel::Cover as never being executed. So I'm thinking either 
Carp::confess() is more severe than what the author actually intended, 
or the author erronously coded to return - could somebody advise which 
so I can make the appropriate change.

In Class::DBI::Iterator::new(), there is no attempt to check the 
validity of the class we a blessing into - hence it is possible to bless 
into main:: if your not careful, and blessing into main:: is probably 
not what you intend. Also the $them parameter is not checked to see that 
it isa('Class::DBI') class, which is important later in next() - if it 
isnt a 'Class::DBI' derived class, then the Class::DBI::construct() call 
in next() will fail - it might be nicer to fail construction rather than 
use. Both these issues are not that important in 'normal' use, there 
just highlighted in the coverage testing. I'd suggest checking that 
ref($me) || $me contains a package name before blessing, and that $class 
has a isa() check done.

Lastly, I've spent a lot of time going cross-eyed at 
Class::DBI::stringify_self(). Devel::Cover reveals several untested 
paths at the line "return (ref($self) || $self) unless $self;" . I've 
come to the conclusion that if called as a method (as intended), the '|| 
$self' is superfluous, and can never be reached - hence I suggest it 
becomes "return ref($self) unless $self;" - I can expand on the tests I 
have written if you need proof. It basically comes down to that if $self 
(from a method call) in bool context fails, then $self must be an object 
and hence ref($self) returns a true value (unless we've been 
particularly psychotic and bless into a package called '0'), hence '|| 
$self' isnt requried. I cant get $self (from a method call) to be false 
in bool context (through _undefined_primary) and not be a reference. You 
can do something like "Class::DBI::stringify_self(0) - which is a 
function call not a method call, and hence is too far removed from 
normal usage - to get path coverage, but I dont think its justified.

Do these sound reasonable ? They add nothing to the feature set of CDBI, 
just janitor the code.

        -- 
        Leif Eriksen
Snr Developer
http://www.hpa.com.au/
phone: +61 3 9217 5545
email: leif.eriksen@xxx.xxx.xx