in reply to RE: new module idea
in thread new module idea

I don't see it on CPAN yet, but gave a quick look at the package from the link above. Quickly, I see you have use strict commented out.. why? You later have no strict 'refs'. Also, please do not put anything on the CPAN which has your email address as a default value. You should not make people set (or unset) default values within the module, ie:

my $def_smtp         = "your.smtp.com";    #<--IMPORTANT!  Set this!
my $def_email         = 'thomasoniii@yahoo.com';
my $def_return        = 'thomasoniii@yahoo.com';
my $def_subject     = "Ye Gods!  An error!";
my $def_domain        = "smtp.com";        #<--IMPORTANT!  Set this! I
+ mean it!
my $def_log_file     = "error.log";

[download]

These things should be done ONLY by passing a hash (I would use a hash) when use'ing the module (or in a new() method, which you don't need to have), or with methods (like Carp::Notify::set_smtp('smtp.me.com'), or Carp::Notify::set_defaults({smtp => 'me.smtp.com', email => 'foo@me.com'}), etc...).
Please change this. 
my $paddr = sockaddr_in(25, $remote_address);

[download]

How do you know it is 25? 
sub today {
...
}

[download]

I like POSIX for things like this, some don't. 
sub error { undef};

[download]

I would have finished this sub before putting on CPAN, personally. You could $Carp::Notify::errlog = 'file' or STDERR (etc...), if Win32 you could log to the Event Log, or make error() a callback to a user defined sub to handle the errors as they wish.

Just some comments from a quick review.

Cheers,
KM

Replies are listed 'Best First'.
RE: RE: RE: new module idea
by jimt (Chaplain) on Aug 14, 2000 at 21:54 UTC
    I don't see it on CPAN yet, but gave a quick look at the package from the link above. Quickly, I see you have use strict commented out.. why? You later have no strict 'refs'.

    I developed the module using strict to make sure that it behaves itself. But once it's done and functional and happy, there's no real need to keep use strict turned on. It is a smidgen bit slower to have 'strict' on, so I keep it off. It can, of course, be turned back on by the individual user. The 'no strict refs' is needed in case someone *does* use strict. If it weren't there, strict would bitch and that's not a good thing, IMHO.


    Also, please do not put anything on the CPAN which has your email address as a default value. You should not make people set (or unset) default values within the module, ie:... These things should be done ONLY by passing a hash (I would use a hash) when use'ing the module (or in a new() method, which you don't need to have), or with methods (like Carp::Notify::set_smtp('smtp.me.com'), or Carp::Notify::set_defaults({smtp => 'me.smtp.com',email => 'foo@me.com'}), etc...). Please change this.

    These values can be set via individual function calls (within explode or within notify), or when the module is use'd. All done via a hash.

    use Carp::Notify (smtp => "smtp.me.com", email => "foo@me.com");

    explode(smtp => "smtp.me.com");

    So the end user can do it however they'd like. Consequently, it's not required to set the defaults. However, most of the time someone will be using the same smtp server, same notification address, same subject, etc. for this module despite which script it is used in. It's nice to be able to set up defaults in one place that can be overridden later if so desired, or left in place if not so desired.

    I like to flag that they should be set in the docs because I really do think that they should so as to avoid headaches later. Best to draw attention to it now than have a difficult-to-debug error later when someone tries to figure out why they can't talk to 'your.smtp.com'

    I know that it's port 25 because I was lazy in it. *shrug* I s'pose I flip it back to be a user-configurable var in a future release.

    I don't particularly like using POSIX (or any other module, if possible) in a CPAN distribution. It really irks me if a module doesn't work out of the box. Nothing against any other modules, just my personal philosophy on keeping my modules self-contained.

    I didn't finish error since I couldn't think of a clever way to do so. It probably doesn't make sense to log that an error occurred to a log file, since one of the ways that error is called is if logging to a file fails. Same argument for having error() email someone with a problem.

    I'll think about adding in the ability to pass function refs, but I'm hesitant because I'm not convinced the end user will fully understand what they're used for. If they hand in a ref that just logs to a file or emails, it probably won't do much good. *shrug*
[reply]
      , there's no real need to keep use strict turned on

      IMO, that isn't good practice, just leave it on. There are two camps on this but when you look at the majority of modules on the CPAN, strict is on and IMO should be left on. If I see a module with it commented out I wonder why.

      So the end user can do it however they'd like

      What happens when a user sets these things in the module itself then you update the module and someone reinstalls it? Poof, their setting are gone. You should not expect people to set anything like that in the module itself. If your module comes with an external config file, fine, but don't expect people to do it within the module.

      I know that it's port 25 because I was lazy in it. *shrug* I s'pose I flip it back to be a user-configurable var in a future release.

      What? And wipe out my default when I upgrade? No thanks ;)

      I don't particularly like using POSIX (or any other module, if possible) in a CPAN distribution

      Some people like it, some people don't. But POSIX comes with Perl, so people have it already. May be overkill for this module, but I like POSIX for various reasons.

      I didn't finish error ...

      I gave some suggestions. If the method is a noop, take it out until you do something with it.

      I'm not convinced the end user will fully understand what they're used for

      So? Then they find out. Your docs should have an example of how to use it (like every other module that uses this technique).

      My biggest nit is that you tell people to set various defaults in the module (and set a few yourself). This has problems, and can lead to later problems (like reinstalling newer versions, and having to edit an installed module to set/change these).

      Cheers,
      KM

[reply]

        > >there's no real need to keep use strict turned on

        >IMO, that isn't good practice, just leave it on. There are two camps on this but when
        > you look at the majority of modules on the CPAN, strict is on and IMO should be left
        > on. If I see a module with it commented out I wonder why.

        I agree - short simple scripts don't need strict but when you have something large, and especially something like a module which may be used by others than yourselves, add the use strict. If it really bothers you, you can always turn it off with a no strict (this is sometimes even necessary) but it is better to make no strict the exception, rather than the rule.

[reply]

In Section Meditations