|Problems? Is your data what you think it is?|
deconstructing String::Iota - not worth one iotaby Anonymous Monk
|on Jun 08, 2009 at 14:06 UTC||Need Help??|
Today I was reading the daily PAUSE uploads and noticed String::Iota. It is a new author's first module. It confirms the proverb: nothing is so bad that it couldn't serve as deterring example.
I would have contributed patches to fix it to bring it to an acceptable level (measured by community standards), but after hacking on S::I for ten minutes I had to give up. Considering the following points 5. to 7. the module is without any redeeming qualities and IMO should be deleted. I name the defects in descending order of importance.
1. There are no tests! That's the single biggest deficiency from which a whole number of other defects stem.
2. Writing tests for coverage reveals that if no explicit parameter is passed, then $_ is used implicitely. Usually that's a very perlish behaviour, e.g. sqrt alone returns the square root of $_. But since both trim functions use several parameters, and the string to be munged is the first parameter (with $_ as fallback), this means the only way to trigger the behaviour on purpose is to pass undef as first parameter. API madness! In code:
Why is that bad? Because the user will not pass undef on purpose; most of the time, the user will pass undef accidently, namely from the value of a yet uninitialised variable. So this is what will happen:
Therefore $_ as fallback in the trim functions is a bad idea and should be removed.
2.a) The real WTF of course is that this behaviour of implicit $_ is not documented at all.
3. There is no parameter validation for the parameter named $num. It really should have one, or else trim will fail silently. There's no consideration for edge cases or invalid input.
4. The trim function is badly named. When experienced programmers think of trim in conjunction with strings, it always means to remove whitespace at the beginning and end of the string. Examples are trim() magic and Text::Trim and autobox::Core for Perl5, S29 (under discussion) for Perl6. I can't think of a better name, mostly because...
5. The trim function can be implemented with literally only substr. The author does not seem to know about this essential string handling function which is part of Perl. Instead we get an inefficient regex-based implementation. I don't see any reason why the single substr call should be wrapped in another function.
trim should be removed altogether.
6. In the documentation, the function signature of strim talks of a delimiter. The synopsis shows a string, twice. The implementation however coerces this to a regex! Again, this surprising behaviour is not documented. But if it would be exposed to the user, passing in any advanced regex as parameter would give unexpected results because under the hood the string is reversed. Again, the implementation is inefficient and duplicates Perl's built-ins.
strim should be removed altogether.
7. I cannot imagine how dismantle is useful. The synopsis teaches:
First element is the length, tail of the list is single characters. So let's take this to its logical conclusion:
Note: there are at least two statements; first the call to dismantle, then some sort of list manipulation or array access to separate dismantle's return value. Why does this deserve its own function? How is this any better than doing directly:
The author does not seem to be aware of context in Perl. dismantle should be removed altogether.
8. Exports trim into caller's namespace. This is even more evil than usual because trim is such a favourite subroutine name among programmers. In other words, it is highly likely that use-ing String::Iota unexpectedly overwrites an existing subroutine. Perversely, strim and dismantle are only avaible with a fully qualified package name: String::Iota::strim(), String::Iota::dismantle(). Better way to export:
9. The synopsis does does not conform to best practices; especially using a bareword filehandle is not modern perl. Using perlcritic guidelines, I rewrote it like this:
Also, the return value of the first strim example ends in a space; this is not discernable from the original comment. Again, writing the tests first and deriving the synopsis from that would have revealed this mistake.
10. The module name is not very explanatory. I know iota as a figure of speech, meaning »a little bit«, but it does not give any hint to what the module actually does with strings.
11. There are POD errors. Previewing with pod2html would have revealed that. Command paragraphs like =head3 need to be followed by an empty line. The function signature should be not be part of the heading, as it is used in HTML for creating the index at the top and anchors in the page, the embedded special characters and mark-up make unwieldy or even unusable permalinks or L<> targets; the signature should be on a separate line. Italic is achieved by I<>, not i<>. Example of better POD:
12. The distribution contains junk files created by the author's operating system (._README, lib/String/._Iota.pm).
13. Spacing is inconsistent, both tabs and spaces are used to create indentation. The command »expand« should be run on the files Changes and lib/String/Iota.pm to change indentation to spaces.
Why do I write this here instead of in an e-mail? Because I do not want to crush a newbie's ego.