in reply to A code review if you please (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:
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: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; }
for (qw/ name prefix server password port channel /) { $attributes{$_} or die "Missing attribute '$_'"; }
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.
|
|---|