Beefy Boxes and Bandwidth Generously Provided by pair Networks
There's more than one way to do things
 
PerlMonks  

deconstructing String::Iota - not worth one iota

by Anonymous Monk
on Jun 08, 2009 at 14:06 UTC ( #769549=perlmeditation: print w/ replies, xml ) Need Help??

Esteemed peers:

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:

# explicit my $str = 'Just another perl hacker'; print trim($str, 4); # 'Just' # implicit $_ = 'Just another perl hacker'; print trim(undef, 4); # 'Just'

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:

# $_ is set somewhere else to a value, e.g. through readline(). # The user programs a bug and $foo is unexpectedly still undef. print trim($foo, 4); # is expected to operate on $foo, but # really returns the first 4 characters of $_

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.

substr 'Just another substr', 0, 4; # 'Just'

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.

# string based my $str = 'Just another index+substr'; substr $str, 0, index $str, 'a'; # 'Just ' # regex based my $pattern = qr/o/; 'Just another zero-width positive look-behind' =~ s/\K$pattern.*//; # 'Just an'

strim should be removed altogether.

7. I cannot imagine how dismantle is useful. The synopsis teaches:

@info = dismantle $str;

First element is the length, tail of the list is single characters. So let's take this to its logical conclusion:

my $length = shift @info; # now do something with $length # now do something with the remaining array of characters

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:

my @elements = split //, 'Just another split on empty regex'; my $length = scalar @elements; # or here, just @elements

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:

use parent qw(Exporter); our @EXPORT_OK = qw( trim strim dismantle );

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:

use String::Iota qw(trim strim dismantle); my $str = 'Just another perl hacker'; print trim($str, 4); # 'Just' print strim($str, 'p'); # 'Just another ' @info = dismantle($str); print join q{, }, @info; # '24, J, u, s, t, , a, n, o, t, h, e, r, , p, e, r, l, , h, a, + c, k, e, r' { open my $fh, '<', 'test.txt'; while (<$fh>) { trim($_, 40); # Trim every line of test.txt to 40 chara +cters strim($_, q{#}); # Get rid of comments } }

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:

=head3 trim C<< trim(I<$string>, I<$num>) >> Trims a string to a certain length, specified by $num.

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.

Comment on deconstructing String::Iota - not worth one iota
Select or Download Code
Re: deconstructing String::Iota - not worth one iota
by JavaFan (Canon) on Jun 08, 2009 at 14:19 UTC
    Considering the following points 5. to 7. the module is without any redeeming qualities and IMO should be deleted.
    No. As with every other module, you have the option to not use it. CPAN is for all, to distribute any (legal) Perl module they want. There's no entrance bar. And it certainly doesn't have to be accepted by some anonymous monk.
Re: deconstructing String::Iota - not worth one iota
by Corion (Pope) on Jun 08, 2009 at 14:25 UTC
    Why do I write this here instead of in an e-mail? Because I do not want to crush a newbie's ego.

    And you think posting this in a public forum is less crushing?

    If you really cared about the author of String::Iota, instead of your ranting you would have taken the time to write a less abrasive text and would have sent the improvements or tips to the module author.

Re: deconstructing String::Iota - not worth one iota
by ikegami (Pope) on Jun 08, 2009 at 15:08 UTC

    this means the only way to trigger the behaviour on purpose is to pass undef as first parameter. API madness! [...] this behaviour of implicit $_ is not documented at all.

    How can something that's not in the API (undocumented) be the basis of a claim of API madness?

    I cannot imagine how dismantle is useful.

    my @elements = split //, $s; my $length = @elements;

    could be written as

    my ($length, @elements) = dismantle($s);

    but then again, you can already write

    my $length = my @elements = split //, $s;

    or

    my $length = my @elements = $s =~ /./sg;

    since list assignment in scalar context returns the number of elements assigned.


    Problems you didn't mention:

    • trim and strim don't work when passed an empty string.
    • trim and strim don't work when passed the string '0'.
    • strim doesn't work if the $delim is more than one character long.
    • strim could be written much better.

    Problems you mentioned I agree with:

    • Allowing undef for the first parameter is unclear, wordy and error-prone.
    • Warnings usually provided by substr are not emitted.
    • The trim function is badly named.
    • The module is poorly named.
    • strim doesn't properly convert the delimiter from text to a regex pattern.
    • trim could be written much better.
    • trim is exported by default.
    • strim and dismantle aren't exported.
    • The synopsis does does not conform to best practices.
    • Badly formatted docs.
    • Functions are of very limited usefulness. It's fine to write a module that has easy to implement functions, but not when they are harder to learn than the building blocks to build the functions in the first place.
Re: deconstructing String::Iota - not worth one iota
by toolic (Chancellor) on Jun 08, 2009 at 15:23 UTC
    I agree with Corion: you should now attempt to send an email to the module author. Now that you have vented here, remove the emotion, and just present the facts. For example, you could debunk the author's claim that the functions are "fast" by providing some reasonable benchmarks against your alternate code.

    If the author does not respond to you in a reasonable amount of time (a week, a month?), start filing bug reports on the CPAN module page for the things that are actual bugs. Include snippets of code to prove the code does not behave as specified in certain circumstances.

    If more time elapses, you can rate the module to share your concerns with other potential users.

    I've noticed that there are no test results for the module. I wonder if it because the module is so new that results are not yet available, or if it simply because there are no tests. In general, if there are no tests results, does that mean there are no tests? In other words, do CPAN testers add some default tests for all modules?

    Just piling on... the "Changes" file lists the version as 0.01, but everywhere else, the version is 0.85. That's probably a typo.

    Update: I looked at the "MANIFEST" link, and there is a single, trivial test (I mistakenly took the OP's "there are no tests" to mean there are zero tests provided in the distribution's "t" directory). So, maybe it'll take another day or so for CPAN test results to appear.

Re: deconstructing String::Iota - not worth one iota
by tweetiepooh (Friar) on Jun 08, 2009 at 16:26 UTC
    At least they've used strict and warnings. And the code is pretty clear and clean.

    But I'd agree with the general sentiment already expressed that an anonymous posting to a public forum can be as "crushing" as a well worded private email, maybe even more so. And while the module is (probably) of limited use and uses less than efficient means to achieve its ends the author has tried. Maybe a more personal contact would have the author revisiting the code, making corrections and learning from the process.

Re: deconstructing String::Iota - not worth one iota
by ruzam (Curate) on Jun 08, 2009 at 18:34 UTC
    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:
    ... open my $fh, '<', 'test.txt'; ...
    Uhmm... What does best practices say about not checking the return value of an open?
Re: deconstructing String::Iota - not worth one iota
by afoken (Parson) on Jun 08, 2009 at 20:08 UTC
    8. Exports trim into caller's namespace.

    In Iota.pm on CPAN dated 08-Jun-2009 00:51, the Exporter module is required and @EXPORT is set, but I can't see any inheritance from Exporter, nor can I see an explicit import of Exporter's import routine. So it DOES NOT export trim. Of course, the code looks like it should export trim.

    Alexander

    --
    Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)
Re: deconstructing String::Iota - not worth one iota
by JavaFan (Canon) on Jun 08, 2009 at 22:02 UTC
    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.
    Of course not. First of all, it's highly unlikely String::Iota is going to widely used. Second, if there's a potential clash, or if people want to avoid a potential clash, people will explicitly import. And once people explicitly import, it doesn't make one iota difference whether the default export set of a module is empty or not. All an empty @EXPORT does is setting up an annoying default (people tend to use (non-OO) modules to its subroutines - therefore, exporting all or a subset of the modules by default makes good sense in many cases). Note also that if you want to use a module, but not want to import any of its symbols, all the caller needs to do is typing two extra characters. Third, so what if there's a clash? It's not that Perl doesn't give a warning the first time you compile the program (and you do have warnings turned on, don't you?). Fourth, if you're going to use a module X which exports routines Y and Z, would you name your own functions Y or Z? Nope, you'd pick something else. People don't just use modules willy-nilly and start reading what it does afterwards.
Re: deconstructing String::Iota - not worth one iota
by CountZero (Bishop) on Jun 09, 2009 at 06:35 UTC
    Isn't this critique better placed on "CPAN Ratings"? That would be the first place I look for to see what the community thinks of a module.

    CountZero

    A program should be light and agile, its subroutines connected like a string of pearls. The spirit and intent of the program should be retained throughout. There should be neither too little or too much, neither needless loops nor useless variables, neither lack of structure nor overwhelming rigidity." - The Tao of Programming, 4.1 - Geoffrey James

Re: deconstructing String::Iota - not worth one iota
by brian_d_foy (Abbot) on Jun 10, 2009 at 17:05 UTC

    Update: I cleaned up my language. I was pretty angry when I first wrote this

    CPAN's philosophy has always been to release early and release often. We want people to upload instead of hiding code in some silo until they think jerks like you will approve and bless it. We want people to put up their work as early as possible so other people can help them with it. Even with my years of experience, I still make some of those mistakes (like the stupid ._ files that show up every time Apple changes the special environment variable for their own tar).

    So what if the module sucks now? People can see it and help the original author improve it. That's the point of open source. If the Perl community had your attitude when I started, I probably wouldn't be involved with Perl at all.

    This is becoming more and more of a problem with the Perl community. Perl Best Practices gave jerks a new way to attack newbies for doing newbie things. I'm genuinely sad when I see a rabid gang of PBP police go after someone who's obviously new. In the big scheme of things, newbies need gentle introductions and guidance when they start, not nitpicking and pedantic attacks about Pod, whitespace, and so on. This sort of crap drives away any of the new blood that Perl could use. It's even driving away some experienced people as the new braindead megaphones crowd out the sensible developers.

    If you really wanted to help, and I don't think you do, you'd pick a couple of important things to help the author with, or even make the patches to fix some of these trivial things. Instead, you simply want to gloat with all your Perlmonks buddies about how you're better than some newbie, all the while hiding beyond Anonymous.

    --
    brian d foy <brian@stonehenge.com>
    Subscribe to The Perl Review

      Update: I cleaned up mine to match.

      I totally agree with the sentiment. I think you overlook the social-newbie angle. Some devs are just socially clumsy, not really jerks. They just don't know how to express the stuff and are in the middle tier of their dev-development. Beginners know they know nothing. Gurus are aware of how much there is to know and most of them replace attitude with confidence. It's that middle ground that sometimes substitutes arrogance for our beloved hubris. Many can be guided out of it with good examples from well-known and highly respected devs. Like say... you. Peer pressure only works on those who want to remain peers. :)

      That said, ++, and I confess to enjoying it when MST unloads some F-bombs on the DBIC list now and then.

      Update: I cleaned up my language. I was pretty angry when I first wrote this

      brian d foy =~ s/d/"devil's tongue"/; :D

Re: deconstructing String::Iota - not worth one iota
by bart (Canon) on Jun 11, 2009 at 10:22 UTC

    There are no tests! That's the single biggest deficiency

    In that case, I have to disagree with your assessment that this module "is not worth one iota". The worst possible fault for a module is that it doesn't make sense as a module. From what I gather from your description, it seems that even you think the concept of this module is sound.

    That makes it good enough for me, as a start.

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlmeditation [id://769549]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others taking refuge in the Monastery: (16)
As of 2014-11-25 21:15 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    My preferred Perl binaries come from:














    Results (158 votes), past polls