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
    i was not quite aware of the indentation.i was just writing the code and was trying to do it the way i can read it..(since it is not done yet, but it works though)..
    i'll just have to change the the indentation later..thanks for your opinion ..i'll try applying your suggestions to the code..

    btw, on the sub new i set
    $self->{INTERFACE},$self->{MACADDRESS} etc. to get default
    values if there are no arguments being passed ...that is why
    i used $self->{MACADDRESS} $self->{INTERFACE} etc (incase the user does not pass any argument)..maybe i'm wrong..kindly check the sub new again please...

      Indentation is not something to fix later. The whole idea is to make code more readable so you don't confuse yourself when you write it and people reading it don't get confused. I suggest you run it through perltidy. The fact that it works has nothing to do with indentation. See obfuscation for unreadable code that works just fine.

      With OO you write accessors and then use them EXCLUSIVELY. The accessors provide the INTERFACE. By doing this you are free to change whatever you want about your internal object representation - so long as the accessors work the same all will be well with any code using your code.

      Debug this:

      my $interface = $obj->interrface; $obj->{INTERRFACE} = $new_interface;
        yeah, the fact that it works has nothing to do with indentation
        what i meant on my previous post was, the scipt works(though, obviously i messed up some things) and i was not referring to indentation..

        sorry to be a pain in the ass...thank you very much for your advice
        i'll read more on acessors..it seems i missed some part of it..hmm..