in reply to Dhcpclient module
I find you code virtually unreadable partly because of the semi-random positioning of the braces (in a style I personally dislike) but mostly because of the definitively random indentation..
Dual function getters/setters have supporters and detractors. I tend to write single function ones like this:
sub get_serverid { $_[0]->{SERVER} } sub set_serverid { $_[0]->{SERVER} = $_[1] } sub get_requestip { $_[0]->{REQIP} } sub set_requestip { $_[0]->{REQIP} = $_[1] } etc
Short and sweet. Note that you don't need return. Perl returns the last value evaluated in a sub automagically. Anyone who knows perl will know what these functions do. Even if you don't read perl they are documenting what they are supposed to do by their very name. While they are marginally less readable on the raw function side they are totally self documenting on the usage side. You can also add error checking to ensure logical arguments are passed to setters.
You can make them longer by doing (and this is standard perl indentation brace positioning BTW):
sub set_widget { my ($self, $arg) = @_; die "Can't set_widget to "$arg"\n" unless $arg =~ m/something/; $self->{WIDGET} = $arg; }
Anyway if you are going to take the time to encapsulate then directly accessing objects like my $state = $self->{STATE}; at the start of create_packet() is a definite no no. my $state = $self->get_state; says it all. You are breaking encapsulation all over the place even though you have get/set methods. $self->{MACADDRESS} (you forgot to code a get/set for that) and $self->{INTERFACE} appear all over the place. They should be $self->get_mac $self->get_interface.....
|
|---|
| Replies are listed 'Best First'. | |
|---|---|
|
Re^2: Dhcpclient module
by drip (Beadle) on Mar 31, 2008 at 01:50 UTC | |
by tachyon-II (Chaplain) on Mar 31, 2008 at 10:05 UTC | |
by drip (Beadle) on Apr 01, 2008 at 02:27 UTC | |
by tachyon-II (Chaplain) on Apr 02, 2008 at 04:54 UTC |