Some (hopefully constructive) criticism on the module's intent, tests and code.
First a few general comments on the intent.
It's a good thing that you are seeing problems and repetitions with your code and are abstracting these out into modules. Carry on doing it. Practice makes perfect.
As I understand it, you're aim is to make it easier to code subroutines that can be called in an OO and in a procedural/functional way. The next few comments are not aimed at your module in particular - but in the concept of having a single subroutine work in both of these ways.
In my opinion the correct solution to getting a subroutine to work in both an OO and functional/procedural way is... "don't do it" :-)
Mixing API styles in the same subroutines in a single module can confuse both module author and user. If you want both styles then write two separate modules, implementing one in terms of the other as appropriate. Simple. Clean. Easy to understand.
If you find yourself mixing OO and functional/procedural interfaces in a single module it's probably a cue that there is something wrong with your design.
An example from recent experience would be my Test::Exception module. The functional API in Test::Exception originally started out as a number of methods in another OO based testing module I was writing (an early precursor of Test::Class if anybody cares). However, by looking at the code I saw that they never actually used the object in question and were a likely prospect to be refactored into a separate module with a functional API... and so Test::Exception was born.
In my opinion a module making mixing OO/functional/procedural styles easier isn't a good idea because mixing OO/functional/procedural styles isn't a good idea.
You may find the discussion around How to morph a plain module to OO of interest.
If you absolutely have to have both implemented by the same subroutines then called_as_method from Devel::Caller is what you need. For example:
use strict;
use warnings;
{
package Foo;
use Devel::Caller qw(called_as_method);
use Carp;
sub new { bless {}, shift };
sub display {
my $self;
# extract $self if called as a method
$self = shift if called_as_method(0);
# check we have even number of key/value pairs
croak "missing key/value (@_)" unless @_ % 2 == 0;
# make a hash of our key/value pairs
my %hash = @_;
# show how we were called
my $type = "subroutine";
$type = (ref($self) ? "object" : "class") . " method" if $self
+;
print "$type called with\n";
while (my ($k, $v) = each %hash) { print "$k = $v\n" };
print "\n";
};
};
Foo->new->display(a => 400, b=>800);
Foo->display(a => 400, b=>800);
Foo::display(a => 400, b=>800);
Foo::display(a => 400, b=>800, 900);
will produce something like:
object method called with
+
a = 400
b = 800
class method called with
+
a = 400
b = 800
subroutine called with
a = 400
b = 800
missing key/value (a 400 b 800 900) at /Users/adrianh/Desktop/foo.pl l
+ine 35
Now I've finished saying evil things about the intent - some comments on the test suite :-)
A round of applause for actually having a test suite. This is something many novice module authors miss out.
However, you are only testing the appearance of your external API (what methods can or cannot be called, whether exported works, etc.). There is not a single test that checks what the various subroutines actually do!
I can remove the contents of every single subroutine and all the tests will pass 100%. This is not usually perceived to be a sign of comprehensive test coverage ;-)
You need to write tests that check what the subroutines are supposed to do. You might find The Joy of Test, What goes in a test suite? and Further adventures with testing of relevance.
You should also consider moving from the Test module to Test::More and the other Test::Builder based modules since they offer a great deal more flexibility and are far easier to use. Very few people use Test in new modules - and usually only in places where the Test::Builder based modules cannot be used.
You also don't need use lib './'; in your test routines. make test will handle this sort of thing for you.
Finally, various comments on the code after a quick review.
This is somewhat a matter of personal taste - but I find your indentation style completely unreadable. Obviously, if it works for you that is fine. However, if you stick to a style that is closer to the norm people will find it easier to read your code. I chucked everything in your distribution through perltidy before I did anything else so I could read it.
Since you don't fully document what your module is supposed to do (with tests, POD or using examples) it is difficult to give useful feedback since it's not easy to see the intent behind all of the code.
For example, I'm not going to comment on shave_opts or coerce_array since I'm not certain of what they are supposed to be doing. I can understand what they do by reading the code. I cannot understand what they are supposed to do without more information. Part of this is down to the naming of the function - shave_opts isn't that descriptive.
This code
ref($_[0])
and
(
ref($_[0]) eq (caller(0))[0]
||
( caller(0) )[0] eq __PACKAGE__
)
Is used twice in different routines and should probably be refactored into its own subroutine. Once And Only Once is a good motto to code by.
There are also several situations where the test won't work the way you want. For example:
- If somebody has an object blessed into package named ARRAY it will get confused with array references (you should use blessed from Scalar::Util instead of ref)
- When you subclass the name of the package a method is called from need not match the name of the package an object was blessed into (you need to use isa not eq).
It also does not cope with situations where you are calling a subroutine in a functional/procedural style - and where the first argument happens to be an object of the appropriate type. called_as_method does.
I don't understand why you reference Getopt::Long and CGI in your documentation - they solve completely different problems.
It's a matter of personal taste (and depends on whether you need to support versions of Perl where our is not supported), but I prefer to use our and use base instead of use vars and @ISA. I find it succinct and easier to read. In this case:
use vars qw( $VERSION @ISA @EXPORT_OK %EXPORT_TAGS );
use Exporter;
$VERSION = 0.01_0; # 12/28/02, 1:54 am
@ISA = qw( Exporter );
@EXPORT_OK = qw( shave_opts coerce_array OOorNO myargs myself );
%EXPORT_TAGS = ( 'all' => [@EXPORT_OK] );
would become
use base qw(Exporter);
our $VERSION = 0.01_0;
our @EXPORT_OK = qw( shave_opts coerce_array OOorNO myargs myself );
our %EXPORT_TAGS = ( 'all' => [@EXPORT_OK] );
None of your code has warnings enabled. Most people consider this to be good style for a couple of reasons:
- 99% of the time the warnings are sensible ones, and you should fix whatever is causing them.
- It can make things difficult for users with versions of perl that do not support use warnings since localising warnings is harder.
If you enable warnings and nothing comes up great - but leave the statements in place and you are protected from any future problems.
Comments like this:
# --------------------------------------------------------
# Constructor
# --------------------------------------------------------
sub new { bless( {}, shift (@_) ) }
and
# --------------------------------------------------------
# Class::OOorNO::Class::OOorNO()
# --------------------------------------------------------
sub OOorNO {
return ( $_[0] )
if defined( $_[0] )
and UNIVERSAL::can( $_[0], 'can' );
}
are redundant. You are just repeating what the code says. You need to add comments when you need to explain why something does what it does, not what it does. In this case we can see by reading the code that new() is the constructor and that OOorNO is a subroutine called OOorNO.
What we cannot tell is what OOorNO is intended to do...
You should remove:
sub DESTROY { }
sub AUTOLOAD { }
from the module and 3_can.t.
The default DESTROY action is to do nothing, so you don't need an empty subroutine to do it for you.
The empty AUTOLOAD definition actually does harm since it will prevent the AUTOLOAD of any superclasses being called, and causes the class to accept any method call without error. We can demonstrate this bug with a simple test script (always write a new test when you find a bug, that way you know when you have fixed it):
use strict;
use warnings;
use Test::More tests => 3;
use Test::Exception;
BEGIN {use_ok 'Class::OOorNO'};
my $o = Class::OOorNO->new;
isa_ok $o, 'Class::OOorNO';
dies_ok {$o->this_method_does_not_exist} 'unknown method dies';
produces:
ok 1 - use Class::OOorNO;
ok 2 - The object isa Class::OOorNO
+
not ok 3 - unknown method dies
# Failed test (t/foo.t at line 13)
+
# Looks like you failed 1 tests of 3.
Remove the AUTOLOAD definition and you get:
ok 1 - use Class::OOorNO;
ok 2 - The object isa Class::OOorNO
ok 3 - unknown method dies
ok
UNIVERSAL::can can take undef as an argument so:
sub OOorNO {
return ( $_[0] )
if defined( $_[0] )
and UNIVERSAL::can( $_[0], 'can' );
}
can be written as
sub OOorNO {
return $_[0] if UNIVERSAL::can( $_[0], 'can' );
}
If the result from OOorNO is only used as a boolean indicating whether something is an object or not then this could be written as:
sub OOorNO {
UNIVERSAL::can( $_[0], 'can' );
}
since the value returned by can will be a subroutine reference, which will be considered true by perl.
In fact, returning $_[0] could sometimes be interpreted as false if $_[0] refers to an object that has cpan://overload|overloaded] bool - so your method will not work in the general case anyway.
I'm guessing (because there is no relevant documentation or test :-) that OOorNO is intended to determine whether its first argument is an object or not. If this is what you intend then you can use the blessed function from Scalar::Util which does exactly that.
Names should not be underestimated. OOorNO does not give many clues as to its intended function. Good naming is a hugely important part of producing easily understandable code. If English is not your first language I understand that this can be hard - but it is still very important and extra time should be spent (consult with an English speaker if you can find one).
I would write shift (@_) as just shift. The fact that shift uses @_ by default in subroutines is well known.
Many people also write bless without brackets. Putting this together with the comment on the use of shift this:
sub new { bless({ }, shift(@_)) }
becomes
sub new { bless {}, shift }
Just do return - rather than return undef or just having undef as your last statement in subroutine. This will return undef in a scalar context and () in list context, which is usually what you want.
Well - that's that from a quick skim over the code. I hope this isn't too discouraging - it's not meant to be. It will probably be worth your time to spend a bit more time on CPAN exploring the modules that are there already. Download them. Look at the code. See how things are layed out.
(and well done for persisting after your initial trial by fire :-)
| 