afresh1 has asked for the wisdom of the Perl Monks concerning the following question:

Hello all, I'm new here and looking for help of course.

I just made my first perl module (WWW::PostiniAdmin) and it seems to work great. That's the problem, although I can generally find errors and fix them. I can't find any glaringly obvious errors with this and I know they are there. I am not yet experienced enough to write perfect code.

I am hoping to get some feedback on my code, documentation, comments and whatever else you see. I didn't want to post a 750 line perl module (most of it pod and comments, honest!), so I am just including links to it.

I would appreciate any help you are willing to give. I won't even be offended if you tell me that I should scrap the whole thing and pay someone to do it for me. However, I would prefer things that will help me learn to be a better perl programmer.

The module is here:
http://www.rraz.net/PostiniAdmin/PostiniAdmin.pm.txt

The HTML from the pod (with links added to the files) is here:
http://www.rraz.net/PostiniAdmin

and you can get the module and some examples in these archives.
http://www.rraz.net/PostiniAdmin/PostiniAdmin-0.05.zip
http://www.rraz.net/PostiniAdmin/PostiniAdmin-0.05.tar.gz

-- andrew

2003-05-02 edit ybiC: retitle from "My first module. I can't find any problems with it!"

  • Comment on Code review request for my first module - WWW::PostiniAdmin

Replies are listed 'Best First'.
Re: My first module. I can't find any problems with it!
by Zaxo (Archbishop) on May 03, 2003 at 01:55 UTC

    A few main points I'd change.

    The name of the module and the hardcoded URI data could and should be generalized. The URI data might be part of the cconstructor arguments, or it could be loaded from a configuration file. That would allow reuse by many sites.

    Second, you have commented out initializations for 'our @ISA' and others. Don't use our for that. @ISA, @EXPORT, @EXPORT_OK, etc. are package globals. Declare with use vars qw( @ISA @EXPORT @EXPORT_OK ); to keep strict happy.

    In the line open DOMAINS, ">Domains.txt" or die #...;, you should use the 3-arg open since you require 5.6.0 anyway, and the 'Domains.txt' should be an absolute path taken from configuration, like the URI data. Remember that a module has even less control than a script over the current directory. Relative paths are made bad things by that.

    It would have been good to post the code directly, between readmore tags. Your links may not always resolve.

    After Compline,
    Zaxo

[reply]
[d/l]
[select]
      I like the idea of reading the URI data from a config file, but I would hate to confuse people by including one. Also, the sections for parsing the input are pretty specific to this site as well, so I am not so sure that pointing it at different URI's would be a Good Thing. I expect to possibly send this in to postini for them to distribute, but I don't see it ending up on CPAN so I think that the specific nature works

      I would probably have figured this part out the part about 'our @ISA' and the rest, but I am not using them, and I got my stubs from h2xs and that is what it generated, so I assumed they were correct. Silly me :-)

      Interesting, I haven't used the 3-arg version before. Kewlness. I will change it to open DOMAINS, '>', 'Domains.txt' or die and use that form in the future!

      Here is the full code of the module:

      Read more... (32 kB)
      andrew
[reply]
[d/l]
[select]
Re: My first module. I can't find any problems with it!
by Limbic~Region (Chancellor) on May 03, 2003 at 01:51 UTC
    afresh1,
    I do not know anything about scraping web pages and I know even less about Postini, but there is something that bothers me about your code: 
    if ($line =~/^&nbsp; &nbsp; &nbsp; &nbsp; <a href/) {

and elsewhere

elsif ($line =~ /<font color=/) {

    [download]
    This implies to me you are depending on the web site never to change their format. That's a pretty hefty risk. You would probably be better off using one of the HTML parsers on CPAN. I can't suggest one, as I indicated I know nothing when it comes to web page scraping, but this search should get you started.

    Cheers - L~R

[reply]
[d/l]
      I did look at dong that, unfortunately there isn't really any good structure to the page beyond what I am matching. The only differences that I can use are the number of &nbsp; there are the links are the same. It is a generated page, and they do notify me before they make changes to their code, so it is a bit of a problem, but I wasn't able to see how parsing the HTML made it any better.

      I am asking for a way to get a comma delimited version, but until then it looks a lot like this:

      Read more... (3 kB)
      -- andrew
[reply]
[d/l]
[select]

Back to Seekers of Perl Wisdom