[Bioperl-l] For CVS developers - potential pitfallwith"returnundef"

Wed May 31 16:44:31 UTC 2006

My feeling is the test suite 'should' pick up a large majority of problems
if changes are made to these lines, the quotes there indicating the utopian
idea that the tests are all written well (I believe 99% of the tests are,
BTW).  You can always try the changes (wholesale or on smaller chunks of
code), see if they pass tests on different OS's using 'make/nmake test',
revert the ones that didn't pass, etc.  It's a matter of someone willing to
try it out.

I think the original argument proposed here (originating from Damian Conway
and 'Perl Best Practices') is maybe using 'return undef' is something we
shouldn't be doing since this can lead to subtle errors itself.  Not that
everything we do is considered 'a good practice' by any means.  If I
remember correctly from 'OOPerl', Conway doesn't like combined get/setters
either (he prefers separate getters and setters); we use the 'bad' combined
version predominately in Bioperl.

Chris

> -----Original Message-----
> From: bioperl-l-bounces at lists.open-bio.org [mailto:bioperl-l-
> bounces at lists.open-bio.org] On Behalf Of Lincoln Stein
> Sent: Wednesday, May 31, 2006 11:03 AM
> To: bioperl-l at lists.open-bio.org
> Cc: Heikki Lehvaslaiho
> Subject: Re: [Bioperl-l] For CVS developers - potential
> pitfallwith"returnundef"
> 
> I'm afraid that everything depends on the context. If the subroutine is
> documented to return a single scalar, then returning undef is appropriate.
> If
> the subroutine is documented to return "false" on failure, then one must
> call
> return (or "return ()" ).
> 
> Changing all the return undefs to return is going to expose hidden bugs in
> the
> code written by people who are using BioPerl. While I agree wholeheartedly
> with the proposed audit, I think we need to expect that people are going
> to
> complain.
> 
> Lincoln
> 
> 
> On Wednesday 31 May 2006 06:55, Heikki Lehvaslaiho wrote:
> > In my opinion the sooner the bugs get exposed the better. It is much
> more
> > likely that there is a well hidden bug caused by assigning accidentally
> > undef into an one element array that someone intentionally writing code
> > that expects that behaviour!
> >
> > I removed (but did not commit yet) all undefs from my old Bio::Variation
> > code and could not see any differences in the test output.
> >
> > Let's remove them!
> >
> > 	-Heikki
> >
> > On Tuesday 30 May 2006 23:40, Chris Fields wrote:
> > > Agreed, though I think these changes should be implemented at some
> point
> > > (Conway's argument here makes sense and it is nice for Torsten to
> check
> > > this out).  If proper tests are written then any changes resulting in
> > > errors should be picked up by checking the appropriate test suite,
> though
> > > I know it doesn't absolutely guarantee it.  ; P
> > >
> > > Chris
> > >
> > > > -----Original Message-----
> > > > From: bioperl-l-bounces at lists.open-bio.org [mailto:bioperl-l-
> > > > bounces at lists.open-bio.org] On Behalf Of Rutger Vos
> > > > Sent: Tuesday, May 30, 2006 1:53 PM
> > > > To: bioperl-l at lists.open-bio.org
> > > > Subject: Re: [Bioperl-l] For CVS developers - potential pitfallwith
> > > > "returnundef"
> > > >
> > > > Although I agree with the sentiment of following PBP, I'm not so
> sure
> > > > changing 'return undef' to 'return' *now* will fix any bugs without
> > > > introducing new, subtle ones.
> > > >
> > > > Chris Fields wrote:
> > > > > Torsten,
> > > > >
> > > > > Any way you can post a list of some/all of the offending lines or
> > > >
> > > > modules?
> > > >
> > > > > Sounds like something to consider, but if the list is as large as
> you
> > > >
> > > > say we
> > > >
> > > > > made need something (bugzilla? wiki?) to track the changes and
> make
> > > > > sure they pass tests; I'm sure a large majority will.
> > > > >
> > > > > I'm guessing Jason would want this somewhere on the project
> priority
> > > >
> > > > list or
> > > >
> > > > > bugzilla, with a link to the actual list, but I'm not sure.  Maybe
> > > > > start
> > > >
> > > > a
> > > >
> > > > > page on the wiki for proposed code changes?
> > > > >
> > > > > Chris
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: bioperl-l-bounces at lists.open-bio.org [mailto:bioperl-l-
> > > > >> bounces at lists.open-bio.org] On Behalf Of Torsten Seemann
> > > > >> Sent: Tuesday, May 30, 2006 3:19 AM
> > > > >> To: bioperl-l at lists.open-bio.org
> > > > >> Subject: [Bioperl-l] For CVS developers - potential pitfall with
> > > > >> "returnundef"
> > > > >>
> > > > >> FYI Bioperl developers:
> > > > >>
> > > > >> I just audited the bioperl-live CVS and found about 450
> occurrences
> > > > >> of "return undef".
> > > > >>
> > > > >> Page 199 of "Perl Best Practices" by Damian Conway, and this URL
> > > > >> http://www.perl.com/lpt/a/2006/02/23/advanced_subroutines.html
> > > > >> suggest:
> > > > >>
> > > > >> "Use return; instead of return undef; if you want to return
> nothing.
> > > > >> If someone assigns the return value to an array, the latter
> creates
> > > > >> an array of one value (undef), which evaluates to true. The
> former
> > > > >> will correctly handle all contexts."
> > > > >>
> > > > >> So I'm guessing at least some of these 450 occurrences *could*
> > > > >> result
> > > >
> > > > in
> > > >
> > > > >> bugs and should probably be changed.
> > > > >>
> > > > >> Your opinion may differ :-)
> > > > >>
> > > > >> --
> > > > >> Dr Torsten Seemann               http://www.vicbioinformatics.com
> > > > >> Victorian Bioinformatics Consortium, Monash University, Australia
> > > > >>
> > > > >> _______________________________________________
> > > > >> Bioperl-l mailing list
> > > > >> Bioperl-l at lists.open-bio.org
> > > > >> http://lists.open-bio.org/mailman/listinfo/bioperl-l
> > > > >
> > > > > _______________________________________________
> > > > > Bioperl-l mailing list
> > > > > Bioperl-l at lists.open-bio.org
> > > > > http://lists.open-bio.org/mailman/listinfo/bioperl-l
> > > >
> > > > --
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > Rutger Vos, PhD. candidate
> > > > Department of Biological Sciences
> > > > Simon Fraser University
> > > > 8888 University Drive
> > > > Burnaby, BC, V5A1S6
> > > > Phone: 604-291-5625
> > > > Fax: 604-291-3496
> > > > Personal site: http://www.sfu.ca/~rvosa
> > > > FAB* lab: http://www.sfu.ca/~fabstar
> > > > Bio::Phylo: http://search.cpan.org/~rvosa/Bio-Phylo/
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >
> > > >
> > > > _______________________________________________
> > > > Bioperl-l mailing list
> > > > Bioperl-l at lists.open-bio.org
> > > > http://lists.open-bio.org/mailman/listinfo/bioperl-l
> > >
> > > _______________________________________________
> > > Bioperl-l mailing list
> > > Bioperl-l at lists.open-bio.org
> > > http://lists.open-bio.org/mailman/listinfo/bioperl-l
> 
> --
> Lincoln D. Stein
> Cold Spring Harbor Laboratory
> 1 Bungtown Road
> Cold Spring Harbor, NY 11724
> (516) 367-8380 (voice)
> (516) 367-8389 (fax)
> FOR URGENT MESSAGES & SCHEDULING,
> PLEASE CONTACT MY ASSISTANT,
> SANDRA MICHELSEN, AT michelse at cshl.edu
> _______________________________________________
> Bioperl-l mailing list
> Bioperl-l at lists.open-bio.org
> http://lists.open-bio.org/mailman/listinfo/bioperl-l