Subroutining for readability...

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

I am trying to write some code that plays nicely with several different directory servers. I am using Net::LDAP. I am modifying several different directories at the same time, what is the preferred method of doing something like that?

#############################################################
## This sub will pass in an entry to be adjusted and a value 
## to adjust with.  It will only work on a single directory  
## and a single task.
#############################################################
sub add_dir_one{
  my ($dn, $entry_to_modify) = @_;

  my $ldap = Net::LDAP->new($directory) 
     or die "Couldn't connect: $@\n";
  my $mesg = $ldap->bind("cn=directory manager, ou=a really long dn", 
+password => "password" ) 
     or die "Could not bind $@\n";;
  $ldap -> modify ($dn, add => {attribute_to_be_adjusted => $entry_to_
+beadded}) 
     or die "Ya screwed up again!";}
}

#############################################################
## This sub will pass in an entry to be adjusted and a value 
## to adjust with.  It will only work on a second directory  
## and a single task.
#############################################################
sub add_dir_2{
  my ($dn2, $entry_to_modify) = @_;

  my $ldap = Net::LDAP->new($directory2) 
     or die "Couldn't connect: $@\n";
  my $mesg = $ldap->bind("cn=directory manager2, ou=a really long dn",
+ password => "password2" ) 
     or die "Could not bind $@\n";;
  $ldap -> modify ($dn2, add => {attribute_to_be_adjusted => $entry_to
+_beadded}) 
     or die "Ya screwed up again!";}
}
[download]

#############################################################
## This sub will pass in an directory name, binding usre/password,  
## action to be taken, entry to be adjusted and a value 
## to adjust with.  It will only work on a single directory  
## and a single task.
#############################################################
sub modify_generic_directory{
  my ($dir, $dir_manager, $pass, $action_to_be_taken, $entry_to_be_adj
+usted, $attribute_to_be_adjusted, $value_to_adjust_with) = @_;

  my $ldap = Net::LDAP->new($dir) 
     or die "Couldn't connect: $@\n";
  my $mesg = $ldap->bind($dir_manager, password => $pass) 
     or die "Could not bind $@\n";
  $ldap -> modify ($entry_to_be_adjusted, $action_to_be_taken => {$att
+ribute_to_be_adjusted => $entry_to_be_adjusted}) 
     or die "Ya screwed up again!";}

}
[download]

and then calling each like this:

&add_dir_one($entry_to_modify, $variable_that_is_adjusting);<br>
&modify_dir_two($entry_to_modify, $variable_that_is_adjusting);<br>
[download]

&modify_generic_directory($dir, $dir_manager, $pass, $action_to_be_tak
+en, $entry_to_be_adjusted, $attribute_to_be_adjusted, $value_to_adjus
+t_with);<br>
&modify_generic_directory($dir2, $dir_manager2, $pass2, $action_to_be_
+taken2, $entry_to_be_adjusted, $attribute_to_be_adjusted, $value_to_a
+djust_with);
[download]

I know there is not a single corect answer, I am just looking for guidelines.

"The social dynamics of the net are a direct consequence of the fact that nobody has yet developed a Remote Strangulation Protocol." -- Larry Wall

Comment on Subroutining for readability... Select or Download Code

Replies are listed 'Best First'.
Re: Subroutining for readability... by runrig (Abbot) on Nov 09, 2001 at 00:15 UTC
I would tend to use the second option. I think explicitly passing in args, having as few globals as possible, and having to write fewer subroutines would be a good thing. But its not the only options. You could go OO and hide all the parameters in, e.g. a hashref and just call the subs as methods, like `$dir_server->modify_directory`. Or you could use closures to write the subs with some fixed parameters for each directory server, and leave the rest as args to pass in. Looking at the docs, the new() method is the only one that returns undef and returns an error message in $@; for the rest, you have to examine the returned Message object. An OO example: package DirServer; sub new { my $class = shift; my ($dir, $dir_manager, $pass) = @_; my $ldap = Net::LDAP->new($dir) or die "Couldn't connect: $@\n"; my $result = $ldap->bind($dir_manager, password=>$pass); $result->code && die "Couldn't bind".$result->error; bless \$ldap, $class; } sub modify_directory{ my $ldap = ${shift()}; my ($action_to_be_taken, $entry_to_be_adjusted, $attribute_to_be_adjusted, $value_to_adjust_with) = @_; my $result = $ldap -> modify($entry_to_be_adjusted, $action_to_be_taken => {$attribute_to_be_adjusted => $entry_to_be_adjusted}); $result->code && die "Ya screwed up".$result->error; } # Then... package main; my $dir_server = DirServer->new(@args); $dir_server->modify_directory(@more_args); [download]	[reply] [d/l] [select]
Re: Subroutining for readability... by Fletch (Bishop) on Nov 09, 2001 at 00:10 UTC
Both have advantages (the first pair has fewer arguments, the second avoids duplicate code). You might consider combining them by having a `_modify_generic_directory` that is called with apropriate arguments by wrapper subs that provide the needed parameters. That way you aren't duplicating the LDAP code in n different subs, but you reduce the amount of parameters callers have to provide. And if you're really feeling OOP-y, you might even go as far as creating your own class that keeps all of the things like the LDAP info and what not in the instance rather than explicitly passing gobs of parameters.	[reply]
Re: Subroutining for readability... by andye (Curate) on Nov 09, 2001 at 16:15 UTC
I'd go for the second option every time. Helps me not primarily with readability, but with think-ability (always at a premium, I find) and debugging. `do_stuff_hide_the_details(); do_other_unrelated_stuff(); sub do_stuff_hide_the_details { do_specific_thing(@these_params); do_specific_thing(@those_params); } sub do_specific_thing { #fiddle about with details }` [download] iyswim! ;) cheers, andy.	[reply] [d/l]