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

Several friends of mine and myself are writing small utility programs for our napster network in the form of "bots" (almost exactly like their IRC equivalent). Since we're all capable perl programmers, I thought it would be best if we could share our code, and move functions from one bot to the other, or write a new function and have it work in another bot. Write once, run everywhere, right?

So I am writing this API for a MP3::Napster bot. Pretty simple code, but I am trying to make it robust and a pleasure to write code for. No use in making something _else_ we dont want to code in.

I'd like it if a few monks could look over the code and make some observations before I plow into the real guts of the API.

Thanks,
brother dep.

package Bot; require 5.6; # update or die. use warnings; use strict; use Carp; use Data::Dumper; use MP3::Napster; use POSIX; ######################################################### ## data, mkay? ## ######################################################### # warnings and debug level, respectively... our ($squawk, $dl); $squawk = 1; $dl = 1; # this is where we are leaving subs we import. our %importedMethods = (); # information on the bot our %botInfo = (); # information for our hackers our %messages = (); # our bot. our $bot; ######################################################### ## subs, mkay? ## ######################################################### # change the debug level sub debug { $dl = shift; } # convert and verify the hash that the programmer gives us for install +ation # into the bot. sub convertAttributes { my %allowedAttributes = ( bot_name => qr/[:alnum:]{1,16}/, prefix => qr/[:alnum:]{1}/, server => qr/[:alnum:]+/, password => qr/[:alnum:]{32}/, port => qr/\d{1,5}/, channel => qr/#?[:alnum:]{1,16}/, ); my %incoming = %{ shift() }; my @badAttributes = grep { $incoming{$_} !~ $allowedAttributes{$_} } keys %incoming; if (scalar @badAttributes == 0) { %botInfo = %incoming; return 1; } else { return 0; } } # handle public messages sub pubHandler { my $botCommands = join '|', keys %importedMethods; $botCommands = qr{($botCommands)}; my $self = shift; my ($ec, $message) = @_; my ($channel, $nick, $msg) = $message =~ m[^(\S+) (\S+) (.*)]; if (my ($command) = $msg =~ m[$botInfo{prefix}($botCommands)]) { # we've been issued a command my $rval = $importedMethods{$command} -> ( $nick, $msg ); if ($rval) { sendPub( $rval ) } } else { return 0 } } # get the bot up and running or die. sub stage { $bot = MP3::Napster -> new ( "$botInfo{server}:$botInfo{port}" ) or die "Could not connect to $botInfo{server}\n"; $bot -> login( $botInfo{bot_name}, $botInfo{password}, 'LINK_UNKNOWN +' ) or die "$botInfo{bot_name} could not log in.\n"; $bot -> join_channel( $botInfo{channel} ) or die "$botInfo{bot_name} could not join $botInfo{channel}\n"; } # verify that the code we are being given is good code. sub checkFuncSyntax { my $codeBlock = shift; { no strict; local $^W = 0; eval "sub {\n$codeBlock\n}"; } die "bad code submitted, $@\n" if $@; return $@ || 0; } ######################################################### ## object constructor, mkay? ## ######################################################### sub new { my $package = shift; my %attributes = %{ shift() }; my ($bot_name, $prefix, $server, $password, $port, $channel); $bot_name = $attributes{name}; $prefix = $attributes{prefix}; $server = $attributes{server}; $password = $attributes{password}; $port = $attributes{port}; $channel = $attributes{channel}; for ($bot_name, $prefix, $server, $password, $port, $channel) { die "Incomplete attribute list" unless $_; } convertAttributes( \%attributes ) or die "Attribute list malformatted"; bless { %botInfo }, $package; } ######################################################### ## methods, mkay? ## ######################################################### # add a new function to the bot. sub addFunction { my $self = shift; my ($funcName, $func) = (@_); if ( my $rval = checkFuncSyntax( $func ) ) { $importedMethods{$funcName} = $func; return 1; } else { warn "function $funcName not imported: $rval\n"; return 0; } } # we do this after we're connected to get the bot running # and install our methods. sub botRun { if (! $bot ) { warn "Bot not connected, or bot not running...\n"; return 0; } $bot -> callback(PUBLIC_MESSAGE, \&pubHandler); $bot -> callback(PRIVATE_MESSAGE, \&msgHandler); return 1; } # send a public message. sub sendMsg { my $self = shift; my $user = shift; my $msg = shift; $self -> private_message( $user, $msg ); return 1; } =cut =pod =head1 synposis my $bot = Bot -> new( server => 'localhost', port => '8888', bot_name => 'bot', password => 'secret', channel = 'bots', prefix => ':' ); $bot -> stage(); $bot -> botRun(); while ($bot) { $bot -> addFunction( 'execute', sub { my $out = qx/shift/; $out } );

--
Laziness, Impatience, Hubris, and Generosity.

Replies are listed 'Best First'.
Re: A code review if you please (code)
by chipmunk (Parson) on Jul 02, 2001 at 23:41 UTC
    Two quick comments on the code:

    There are two subtle problems with this statement:

    require 5.6;
    The first is that it will require perl v5.600.0 rather than v5.6.0, because 5.6 is a plain old number rather than a version string. The second is that it is executed at runtime, while you are using compile-time features of 5.6.0 such as our and the warnings pragma. Change that line to: use 5.006; to get the proper behavior in all versions of Perl.

    Next, I'm confused by your constructor:

    sub new { my $package = shift; my %attributes = %{ shift() }; my ($bot_name, $prefix, $server, $password, $port, $channel); $bot_name = $attributes{name}; $prefix = $attributes{prefix}; $server = $attributes{server}; $password = $attributes{password}; $port = $attributes{port}; $channel = $attributes{channel}; for ($bot_name, $prefix, $server, $password, $port, $channel) { die "Incomplete attribute list" unless $_; } convertAttributes( \%attributes ) or die "Attribute list malformatted"; bless { %botInfo }, $package; }
    I can't figure out why you copy all those attributes out of the hash into lexical variables that go out of scope at the end of the subroutine anyway. If you want to verify the attributes, just do it directly on the hash values:
    for (qw/ name prefix server password port channel /) { $attributes{$_} or die "Missing attribute '$_'"; }

    And one important comment on the design:

    There seems to be something seriously amiss with the design of this module. You've got an object constructor and object methods, but you're storing instance data in global variables (i.e. $bot and %botInfo)! What happens if you create two objects of this class? I'm not sure, but I expect it would be messy...

    Currently, the interface and the implementation are not consistent. I think that's the first thing to work on for this module.

Re: A code review if you please (code)
by japhy (Canon) on Jul 02, 2001 at 23:15 UTC
    I'll spend more time on this later, but I need to point out now that the regexes that use [:alnum:] are broken.
    qr/[:alnum:]/; # /[almnu:]/ qr/[[:alnum:]]/; # /[[:alnum:]]/
    See? Also, why the POSIX class and not its simpler \w equivalent?

    japhy -- Perl and Regex Hacker
Re: A code review if you please (code)
by Brovnik (Hermit) on Jul 03, 2001 at 13:33 UTC
    Evalling code sent to you by someone else looks dangerous to me.

    Are you going to do any checking ?

    Presumably you trust those sending code to you, but what happens one day when a virus gets through ?

    Maybe you should consider some sort of verification with a certificate.

    How about using e.g. PGP or GnuPG to sign code snippets. You could then only import code from those on your keyring. Add those you trust to the keyring.

    The code to send a new code snippet could do the signing for you almost (except for the passphrase) invisibly.
    --
    Brovnik