|Think about Loose Coupling|
Handy dandy CPAN pollutionby Juerd (Abbot)
|on Dec 26, 2002 at 01:29 UTC||Need Help??|
It's Christmas. We're supposed to be nice to eachother now. Warning: this is not a nice post. If you want a happy Christmas, read it later :)
Every day I check http://search.cpan.org/recent to check if my favourite modules have newer versions, and to find out about interesting new ones. I forgot to check yesterday, but a module named Handy::Dandy was released just that day. I clicked the link to find out exactly how handy this module is.
Well, it was rather disappointing. No, I didn't contact the author first. If you release something on CPAN, you should communicate beforehand. I seriously doubt Tommy Butler did.
I must admit that I didn't test or even let perl interpret the code. That's because I'm not installing this one; I don't want code like this on my system. Sorry, mister Butler.
The author introduced a new root namespace: Handy::. This is a big mistake. All modules are supposed to be handy, so why announce that your module is handy? Clearly he doesn't understand CPAN. Exactly the same mistake was made with Dandy. Especially since this module is not handy, and not dandy.
Version numbers are important for many things. The distribution is version 1.306.2, but the actual $VERSION is 1.30_6. The $VERSION did not change since the previous release.
I always like to know what has changed since the previous release. But this Changes file only lists the initial version. Too bad.
Well, that's what the author seems to think. He used OOorNo, which he also wrote himself. A useless module that gives you your arguments without the first one if that's a class or object. This has nothing to do with object orientation, it's just some very strange and useless syntactic sugar: you can now use Package->function and $object->funtion instead of Package::function. Yay?
Yeah, good question. Where is it? There is some pod in the module, but it tells you nothing more than the code does. It lists @ISA, @EXPORT, @EXPORT_OK, %EXPORT_TAGS and the available functions ("methods"). The functions are not documented. There's not even a hint about what arguments they expect. Or about what they return. If you want to use any of these "simple, reusable code routines", you will have to read the module's source.
So I did. It's horrifying. I'm more or less documenting the module now.
This function does nothing. Maybe it should have been called use_not. An empty block. What's the use?
This is shorthand for the combination of the next two functions. It passes @_ entirely (explicitly), but since isint and isfloat need a single argument, this one does too.
This function takes one argument, and returns that argument if it matches ^\d*\z, undef if not (or not defined). This means a false value is being returned if you give it 0. I thought 0 was an integer.
I think the regex I used was too simple or something. Mister Butler tries to impress the reader with a nice tr, but he doesn't understand how it works.
As you can see, there's a double negation: !~ and /c. Useless obfuscation. And /d is being used to delete found but unreplaced characters. But since the explicitly written 0-9 is repeated in the RHS, nothing actually is deleted. Not that any deletion is needed: the variable is thrown away.
There's 5 lines of explanation comments about why underscore is not handled at all. Nothing is said about periods. I can live with 3.0 being an integer and it being not an integer, as long as the reason is documented.
I'd probably use a simple int $foo == $foo.
The first argument is used, all others are discarded. The function returns undef when it thinks the value you give it is not a floating point number, or the number itself when it thinks it is. This function too is loaded with tr vagueness.
If the stringified version of the value to check does not contain a dot, it's not a float. This means isfloat(1.0) returns undef, while isfloat("1.0") returns 1.0.
Any string ending in a dot, or having more than one dot is not a float. This means 1. is neither an integer nor a float, according this module. That also means it's not a number. Perl can handle this format, though.
Again, perlfaq4 has a much better solution.
Oh, I almost forgot to mention that because isint returns the number for integers, and because isnum uses a truth check for what isint returns, and because isfloat rejects strings without a period, this module doesn't regard zero as a valid number.
(I'm disregarding unicode here.)
The author chose to split the string passed it into separate characters, and then iterate over that array to implement s/([\x21-\x2F\x3A-\x64\x91-\x60\x7B-\xFF])/'&#' . ord($1) . ';'/ge;.
That's right: non-printing characters are left alone. Note that of the 33 ASCII characters that this function substitutes for entities, only four actually need to be: &, <, > and ".
is written as \046\043, ; is written as \073. I guess this is just to impress people, to show off his l33tness.
HTML::Entities is a much better module for this, even though something as simple as s/([^\x20-\x7E])/'&#' . ord($1) . ';'/ge works fine.
A temporary hash is used to see if the first argument is equivalent to any of the other arguments. The hash values are equal to the keys, what a waste of space. Besides, the entire list is copied a few times. All of this just to be able to write isin($foo, @bar) where grep $_ eq $foo, @bar would probably have sufficed.
If the first argument is false, this function returns undef. This means you cannot check if the list contains undef, an empty string or zero (string or number).
String comparison is used, because hash keys can only be strings. isin(5.0, @foo) will find 5 in @foo, but not 5.0. There's no way to get this function to use numeric comparison.
This is used as convert_size($foo, 'bytes to megabytes'). It only supports bytes (/^by/), kilobytes (/^ki/) and megabytes (/^meg/), and cannot handle extra whitespace. The second word of the second argument is ignored.
This function made me extremely sad. Dear Tommy, a kilobyte is not 1000 bytes, it is 2**10 == 1024 bytes. A megabyte is not 1000000 bytes, it is 1024 * 1024 == 1048576 bytes. All computer users, but especially coders should know this!
I'm skipping this, because I know nothing about utf8. I assume the function is broken, but lack clue to prove it. Update - It has nothing to do with utf8. At all. This function is just a miserable attempt to re-implement what URI::Escape does best.
This does what html_escape does, but with all characters. This function too splits, iterates and joins where a very simple regex would also have functioned (and have been much faster).
Do not use this module.
To Tommy Butler
You are a fool, but you probably realised that by now. You put your broken code on CPAN, violating lots of written and unwritten rules.
Please remove your broken modules from CPAN immediately. People must not use this, as it will very probably break their programs, giving them a bad impression of Perl.
Please learn some Perl before uploading another module to CPAN. Make sure you understand what you write completely. If you do not understand something, ask for help. The Perl community is a friendly and helpful one (although this review is not at all nice). When you know Perl, you probably would not even think about creating a module like this.
Read what the CPAN guides say about communication. Post your module somewhere and request comments.
To PerlMonks readers
Downvote me if you want to. I see no reason to be nice to this author. He polluted CPAN, and didn't read perlfaqs, didn't read http://pause.perl.org/pause/query?ACTION=pause_04about. People like Butler give Perl and CPAN a very bad name.
If you're learning Perl, a good exercise is to read http://search.cpan.org/src/TOMMY/Handy-Dandy-1.306.2/Dandy.pm and find out where the bugs are. There are plenty and I expect even beginners to be able to find some.
Sorry if this ruined your Christmas.