in reply to RFC: Module - GSM::Nbit

I don't see any big problems at all. Here are some minor suggestions for improvements.

I believe you have a typo in your warn message: change 'characted' to 'character'.

You should indent the code in the 'Code Sample' section of the SYNOPSIS in your POD. When I use perldoc, it renders poorly for me. You should also remove the tab characters in that code example because it still renders poorly even after I indented it.

You should add more data validation for your input. If I do something that you probably don't expect, I do get lots of warnings, but they could be more helpful. I tried this (probably) illegal thing, and I got about 20 warnings: 

my $foo = $gsm->decode_7bit_wlen(5);

[download]
Of course, that means you should add tests to your test suite. The Perl Best Practices book has a nice, succinct checklist of general-purpose things to test.

As a user, I absolutely love it when a CPAN author includes a script I can run out-of-the-box, preferably in the bin directory, or at least in the examples directory. This could be nothing more than the code from the SYNOPSIS.

There are plenty more style suggestions if you run the code through perlcritic.

Update: Do this before you upload your tar file: Fix CPAN uploads for world writable files

You should delete the INSTALLATION section from the POD. There is no need to tell someone how to install something which has already been installed.

Another typo: change 'costructor' to 'constructor'. I guess you should run your code through a spell-checker.

Replies are listed 'Best First'.
Re^2: RFC: Module - GSM::Nbit
by techcode (Hermit) on May 14, 2010 at 06:05 UTC

    Yeah - there are some typos as I still need to run it through a spell checker (and perlcritic) - any tools for that?

    EDIT: perlcritic does a nice job of spell checking as well :)

    I had the code indented, but then in man pages it looked like double indented from being inside a section + code indentation itself. But I agree it looks better that way so reverting + removing spaces between variable, = and value.

    Yes for more data validation - I also "unified" so everything just does "return unless xyz;" instead of having both that and "return '' unless xyz;" in a few places.

    Agree on examples directory :)

    As of world writable files - that would be a bugger to figure out on my own. But looking at my folder structure I have standard chmod 644 for files and 755 for folders. So I'm not sure what's wrong?

    First point I don't completely agree on - I believe INSTALLATION section is there for the CPAN web page?

    Thanks!

    Have you tried freelancing/outsourcing? Check out Scriptlance - I work there since 2003. For more info about Scriptlance and freelancing in general check out my home node.
[reply]
      As of world writable files - that would be a bugger to figure out on my own. But looking at my folder structure I have standard chmod 644 for files and 755 for folders. So I'm not sure what's wrong?
      To be honest, I do not understand why this is necessary. But, I do know that one of my uploaded tar files was rejected by CPAN testers because of a permission problem. When I ran bart's script on my tar file, the problem disappeared. I have used it on every upload since, and I have not had a repeat of the problem. It's easy to run, and it helps avoid hassles. Why not just do it?
      First point I don't completely agree on - I believe INSTALLATION section is there for the CPAN web page?
      INSTALLATION belongs in the README file, and you already have it there. I see no reason to duplicate it in your POD. Look at the POD for your 10 favorite modules. Do you see an INSTALLATION section in any of them? 
      perldoc List::MoreUtils

      [download]
[reply]
[d/l]
[reply]

In Section Seekers of Perl Wisdom