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

G'day, I'm having problems with two subroutines using the same variables, and I'm not sure how to proceed.
When I declare the variables globally, it all works, but I have been told this is not the Right Way.
The code is as follows: 
#!/usr/bin/perl -w
use strict;
use warnings;
use diagnostics;
use Net::LDAP;

my $givenname="Neil";
my $sn="Hunt";
my ($ldap,$mesg);

&ldap_search;

sub ldap_search {
        &ldap_bind;
    my $base="ou=People,dc=btester,dc=com,dc=au";
    $mesg = $ldap->search (
        base => $base,
        filter => "(&(cn=$givenname $sn))");
    if ($mesg->count == 0) {
        print "Hi!\n";
    }
    else {
        print "Content-type: text/html\n\n";
        print "<title>Already Exists</title>";
        print "$givenname $sn already exists";
        print "<p>";
        print "Go <a href=\"../addemp.html\">Back</a>";
    }
}

sub ldap_bind{
    my $ldap_server="argyle";
    my $user="cn=Directory Manager";
    my $pass="<password>";
    return $ldap = Net::LDAP->new( $ldap_server ) or die "$@";
    return $mesg = $ldap->bind( "cn=Directory Manager",
                     password => "secretsecret"
                   );

}

[download]
This code is part of a larger programme which is for adding and updating users in LDAP. The ldap_bind() sub needs to be called multiple times depending on the situation the script is in.

Update: Thanks Zaxo. I applied the changes you suggested. Not only does my code work nicely, I learnt so much more about how a subroutine works.
Thanks!

Replies are listed 'Best First'.
Re: Passing parameters between subroutines
by Zaxo (Archbishop) on Aug 10, 2004 at 02:24 UTC

    First thing is that ldap_bind() is returning in the next-to-last statement, so $ldap->bind is never actually called.

    The better alternative to sticking values in global variables is to pass needed parameters to each sub and return what each sub produces. The mechanism is simple. Perl subs receive their arguments in the @_ array, and they return either the arguments of the first return statement they encounter, or else the value of the last statement evaluated.

    Here is a minimal modification to your code to follow those principles, 

    #!/usr/bin/perl -w
use strict;
use warnings;
use diagnostics;
use Net::LDAP;

ldap_search( qw/Neil Hunt/);

sub ldap_search {
    my ($givenname, $sn) = @_;
    my ($ldap, $mesg) = ldap_bind();
    my $base="ou=People,dc=btester,dc=com,dc=au";
    $mesg = $ldap->search (
        base => $base,
        filter => "(&(cn=$givenname $sn))");
    if ($mesg->count == 0) {
        print "Hi!\n";
    }
    else {
        print "Content-type: text/html\n\n";
        print "<title>Already Exists</title>";
        print "$givenname $sn already exists";
        print "<p>";
        print "Go <a href=\"../addemp.html\">Back</a>";
    }
    1;
}

sub ldap_bind{
    my $ldap_server="argyle";
    my $user="cn=Directory Manager";
    my $pass="<password>";
    my $ldap = Net::LDAP->new( $ldap_server ) or die "$@";
    my $mesg = $ldap->bind( "cn=Directory Manager",
                     password => "secretsecret"
                   );
    return ($ldap, $mesg);
}

    [download]
    Doing that is likely to point up places to improve your code. I was tempted to make $ldap_server, $user, $pass arguments to ldap_bind, but it seemed just as likely that the sub was a desired specialization and that no fixing was needed.

    Your code (and my modified version) really needs some error checking code added. You don't know whether some of your LDAP operations have succeeded or not.

    After Compline,
    Zaxo

[reply]
[d/l]
      Thanks Zaxo for your wisdom. Neil
[reply]

Back to Seekers of Perl Wisdom