[perl] Writing my first module (IPTables::Rule); Looking for feedback

fukawi2 · 2011-12-24 04:11:00

Well pretty much as the subject says... I've been hacking away with perl for a while, but this is the first time I've tried writing a module.

I think (hope) I'm on the right track in terms of doing it the "right" way (or one of the X "right" ways perl lets you!).

If you're bored and have time to have a glance over and tell me how I'm doing, that would be great

https://github.com/fukawi2/IPTables-Rule

juster · 2011-12-24 22:11:36

This looks nice and seems to be very complete. Creating modules is a thankless art and I wish you the best. I skimmed through it and noticed a few things that stood out to me:

You use empty prototypes for all your functions. These are not necessary and I think you have picked up this habit from somewhere else. Most likely from shell scripting. I also only mention this because prototypes can be misleading to programmers because they almost always do the unexpected and cause subtle confusion.

Prototypes are specified in parenthesis after the sub name definition of a sub. Inside the parenthesis is a specification on what arguments are expected. Prototypes provide a hackish sort of argument-checking to perl. Their original purpose was to allow passing arrays to functions, like the perl builtins push, splice, etc. They do more and can be useful for things like DSLs but they usually only confuse beginners. Your empty prototypes specify that the subroutine takes no arguments. See the Prototypes section of the perlsub manpage for more: http://perldoc.perl.org/perlsub.html#Prototypes

Off the top of my head I think that object methods do not check to make sure arguments match prototypes. So all of your methods will work fine, even though you have specified that your subs take no arguments, in the empty prototypes. I see you call private functions, preceded by the ampersand (&). This is the old perl 4 syntax for calling functions. Luckily this ugliness is behind us. The only practical reason to use it now is to disable argument checking which has been previously enforced by prototypes. If you remove the empty prototypes you won't have to call your "private" subs by prefixing their names with ampersands.

The setters in your docs use assignment instead of passing the new values as arguments to methods. For example:

=head3 mac

Source MAC Address to match against.

  $ipt_rule->mac = '6c:f0:49:e8:64:2a';

This is a little confusing to me. Perhaps just a typo or the old way of doing things? I also like moving my docs into a separate .pod file (i.e. Rule.pod) when they get very long.

Lastly, and maybe most important, why not put this on CPAN? Join us in the chaos! Anyways... have fun and merry xmas.

fukawi2 · 2011-12-24 23:44:27

juster wrote:

This looks nice and seems to be very complete. Creating modules is a thankless art and I wish you the best. I skimmed through it and noticed a few things that stood out to me:

Thanks for your time and input

juster wrote:

You use empty prototypes for all your functions. These are not necessary and I think you have picked up this habit from somewhere else. Most likely from shell scripting. I also only mention this because prototypes can be misleading to programmers because they almost always do the unexpected and cause subtle confusion.

Sure, let's say shell scripting............
Removed them all.

juster wrote:

I see you call private functions, preceded by the ampersand (&). This is the old perl 4 syntax for calling functions. Luckily this ugliness is behind us. The only practical reason to use it now is to disable argument checking which has been previously enforced by prototypes. If you remove the empty prototypes you won't have to call your "private" subs by prefixing their names with ampersands.

I was actually aware the ampersands are not required, but I like them... They're just a little hint when skimming code that it's a function... Just like $, @ or % on variables. Having a Google around, I didn't actually know there was a difference between using it and not using it, so I'm removed them too. I'll get used to it

juster wrote:

The setters in your docs use assignment instead of passing the new values as arguments to methods.

Doh, that's why an extra set of eyes is useful... The test module can't pick that up
Fixed also.

juster wrote:

Lastly, and maybe most important, why not put this on CPAN? Join us in the chaos! Anyways... have fun and merry xmas.

I was thinking about it... Wanted to make sure it's not a complete dogs breakfast first though

Arch Linux

#1 2011-12-24 04:11:00

[perl] Writing my first module (IPTables::Rule); Looking for feedback

#2 2011-12-24 22:11:36

Re: [perl] Writing my first module (IPTables::Rule); Looking for feedback

#3 2011-12-24 23:44:27

Re: [perl] Writing my first module (IPTables::Rule); Looking for feedback

Board footer