Problems passing a reference to a hash between functions

[niall.macpherson]

I have a sub which calls the fetchall_hashref method from the DBI
module. I like to have all my subs return either 0 (success) or -1
(failure) and if the sub need to return some data to the caller I
always pass a variable by reference and populate that variable . I tend
to try to think of refs the same way as I did with pointers in 'C'
which I am more familiar with. What I am trying to do here is pass a
reference to a reference to a hash to the sub.

I do not want to retrun anything by value as potentially I could be
dealing with some very large hashes here

Normally passing references works fine - e.g the Connect() sub here
takes a ref to a scalar and populates it with the connection handle so
this value can then be passed to another sub. However I am having real
problems with the GetTableValues() sub. This calls
$sth->fetchall_hashref() from DBI which returns a reference to a hash.
I can't seem to get this back to my main code body so that I can then
pass it to the ProcessTableValues() sub . The current code compiles and
produces no errors but ProcessTableValues() produces no output.

I'd would be grateful if someone could tell me what I am doing wrong .

Thanks

##-------------------------------------------------------------------
## sqlhashref.pl
## Test passing ref to hash between subs
use strict;
use warnings;
use File::Spec::Functions;
use Data:umper;
use DBI;

##------------------------------------------------------------------
sub Connect
{
## Works as expected
my ($dsn, $r_conn) = @_;

my %attr = ( RaiseError => 1, AutoCommit => 0);
$$r_conn = DBI->connect("dbi:ODBC:" . $dsn , undef, undef, \%attr);

if(!defined($$r_conn))
{
print STDERR "\n", 'Failed to connect ' , $DBI::errstr;
return(-1);
}
return(0);
}
##------------------------------------------------------------------
sub GetTableValues
{
## Cannot seem to get reference to results back to main section
my ($conn, $rrh_results) = @_;

my $rh_results = undef;
my $sth = $conn->prepare('select tabid, tabname from systables where
tabid < 5');
if(!defined($sth))
{
print STDERR "\n", 'Failed to prepare ' , $DBI::errstr;
return(-1);
}
$sth->execute();
$rh_results = $sth->fetchall_hashref('tabid');

$$rrh_results = $rh_results;

return(0);
}
##------------------------------------------------------------------
sub ProcessTableValues
{
my($rh_results) = @_;

foreach my $key (keys %{$rh_results})
{
print "\n", $key, $rh_results->($key);
}
return(0);
}
##------------------------------------------------------------------
my $conn = undef;
my %results = ();
if(Connect('sysmaster', \$conn) < 0)
{
exit(-1);
}
if(GetTableValues($conn, \\%results) < 0)
{
$conn->disconnect;exit(-1);
}
#print Dumper %results; - No output produced by this print
if(ProcessTableValues(\%results) < 0)
{
$conn->disconnect;exit(-1);
}
$conn->disconnect;
exit(0);

 

[Paul Lalli]

I have a sub which calls the fetchall_hashref method from the DBI
module. I like to have all my subs return either 0 (success) or -1
(failure) and if the sub need to return some data to the caller I
always pass a variable by reference and populate that variable . I tend
to try to think of refs the same way as I did with pointers in 'C'
which I am more familiar with.

That's probably a mistake. They're similar, but not equivalent. Don't
try to program C in Perl. Program Perl.

What I am trying to do here is pass a
reference to a reference to a hash to the sub.

You're going through way too much trouble. Subroutine arguments in
Perl are already passed by reference. You just have to use them as
references.

sub GetTableValues
{
## Cannot seem to get reference to results back to main section
my ($conn, $rrh_results) = @_;

Don't copy the parameter into a new variable. Use it directly.

my $rh_results = undef;

That initialization serves no purpose. All scalar variables are undef
by default.

my $sth = $conn->prepare('select tabid, tabname from systables where
tabid < 5');
if(!defined($sth))
{
print STDERR "\n", 'Failed to prepare ' , $DBI::errstr;
return(-1);
}
$sth->execute();
$rh_results = $sth->fetchall_hashref('tabid');

$_[1] = $sth->fetchall_hashref('tabid');

$$rrh_results = $rh_results;

Eliminate this mess entirely.

return(0);
}

my $conn = undef;
my %results = ();

Declare a reference to a hash, rather than an actual hash:

my $results_ref = { };

if(Connect('sysmaster', \$conn) < 0)
{
exit(-1);
}
if(GetTableValues($conn, \\%results) < 0)

GetTableValues($conn, $results_ref)

The hash reference you pass here will be equivalent to $_[1] in the
subroutine, so when the subroutine sets $_[1], your changes will be
seen in $results_ref after the call.

{
$conn->disconnect;exit(-1);
}
#print Dumper %results; - No output produced by this print
if(ProcessTableValues(\%results) < 0)
ProcessTableValues($results_ref)

{
$conn->disconnect;exit(-1);
}
$conn->disconnect;
exit(0);

Hope this helps,
Paul Lalli

 

[niall.macpherson]

Paul Lalli wrote:

You're going through way too much trouble. Subroutine arguments in
Perl are already passed by reference. You just have to use them as
references.

Declare a reference to a hash, rather than an actual hash:

my $results_ref = { };

Hope this helps,
Paul Lalli

Thanks Paul - those 2 points are the fundemental issues I was missing -
it certainly does help !

 

[Uri Guttman]

PL> That's probably a mistake. They're similar, but not equivalent. Don't
PL> try to program C in Perl. Program Perl.

amen.

PL> You're going through way too much trouble. Subroutine arguments in
PL> Perl are already passed by reference. You just have to use them as
PL> references.

i would disagree with this. direct access via @_ can be very confusing
to others. it is better to pass in a reference to the result variable as
the OP has done. his real problem was the bug you corrected.

PL> $_[1] = $sth->fetchall_hashref('tabid');

doing that so late in a sub is potentially very confusing and could even
be a bug waiting to happen if someone later on messes with @_ by
shifting it or something. you can also get that reference by doin \$_[1]
and storing that into a my variable at the top of the sub. then you can
assign the result via dereferencing that variable. at least that way all
the work on @_ is done at the top of the sub.


PL> Declare a reference to a hash, rather than an actual hash:

PL> my $results_ref = { };

PL> GetTableValues($conn, $results_ref)

PL> The hash reference you pass here will be equivalent to $_[1] in the
PL> subroutine, so when the subroutine sets $_[1], your changes will be
PL> seen in $results_ref after the call.

even simpler would be to just pass in a regular hash ref and use it
inside the sub and assign to it. the ref of a hash ref is overkill and
your method is not common at all.

# you can do this with a hash or a hash ref. you just need to pass in
# the ref directly instead

my %hash ;
# or my $hash_ref = {} ;

foo( \%hash ) ;
# or foo( $hash_ref ) ;

sub foo {

my( $result_ref ) = @_ ;

%{$result_ref} = %{$sth->fetchall_hashref('tabid')} ;

}

yes that does a full shallow copy of the hashed row each time. but the
code is much simpler and that means something too.

so you can instead store the dbi row hash ref as an element of the
return ref like this:

$result_ref->{row} = $sth->fetchall_hashref('tabid') ;

and access that from the calling code with:

$hash{row}
# or $hash_ref->{row}

this leads to the direction of OO since that is how you might do it in
OO. the method would store the results (and error info and such) in the
object and also return an error code if that is the API.

too many ways to do this so pick your favorite flavor.

uri

 

[Paul Lalli]

PL> $_[1] = $sth->fetchall_hashref('tabid');

doing that so late in a sub is potentially very confusing and could even
be a bug waiting to happen if someone later on messes with @_ by
shifting it or something. you can also get that reference by doin \$_[1]
and storing that into a my variable at the top of the sub. then you can
assign the result via dereferencing that variable. at least that way all
the work on @_ is done at the top of the sub.

I wholeheartedly agree with you here. I hadn't thought to save off a
copy of the reference at the beginning. Thanks for pointing that out
to me.

sub foo {
my( $result_ref ) = @_ ;
%{$result_ref} = %{$sth->fetchall_hashref('tabid')} ;
}

yes that does a full shallow copy of the hashed row each time. but the
code is much simpler and that means something too.

Very true. However, you snipped my reasoning for not doing this to
begin with. The OP specifically said:

That's the sole reason I was attempting to go through the trouble of
directly modifying $_[1], rather than doing any copying. All other
things being equal, I completely agree with your solution above.

Paul Lalli

 

[Uri Guttman]

PL> Very true. However, you snipped my reasoning for not doing this to
PL> begin with. The OP specifically said:

PL> That's the sole reason I was attempting to go through the trouble of
PL> directly modifying $_[1], rather than doing any copying. All other
PL> things being equal, I completely agree with your solution above.

but my later solution is better yet as it doesn't copy the hash and
still needs only a simple hash ref to be passed in and with no special
@_ hacks. just return the DBI row ref as an element of the result hash
ref. best of all worlds there. simple ref passing, no deep copy.

uri

 

[Paul Lalli]

PL> That's the sole reason I was attempting to go through the trouble of
PL> directly modifying $_[1], rather than doing any copying. All other
PL> things being equal, I completely agree with your solution above.

but my later solution is better yet as it doesn't copy the hash and
still needs only a simple hash ref to be passed in and with no special
@_ hacks. just return the DBI row ref as an element of the result hash
ref. best of all worlds there. simple ref passing, no deep copy.

Ahhh, I missed that. Or mis-parsed it, or something, I didn't notice
what you were doing after the paragraph I had commented on. Very good
then. In every way a better solution than mine.

Thanks, Uri.

Paul Lalli

 

[Tad McClellan]

What I am trying to do here is pass a
reference to a reference to a hash to the sub.

But you don't need to pass a reference to a reference to a hash to the sub.

You only need to pass it a scalar, which will be set to a reference
to a hash in your subroutine.

I do not want to retrun anything by value as potentially I could be
dealing with some very large hashes here

This calls
$sth->fetchall_hashref() from DBI which returns a reference to a hash.

How convenient, you need a reference to a hash, and fetchall_hashref()
gives you a reference to a hash. What is the problem again?

##------------------------------------------------------------------
sub GetTableValues
{
## Cannot seem to get reference to results back to main section
my ($conn, $rrh_results) = @_;

my $rh_results = undef;
my $sth = $conn->prepare('select tabid, tabname from systables where
tabid < 5');
if(!defined($sth))
{
print STDERR "\n", 'Failed to prepare ' , $DBI::errstr;
return(-1);
}
$sth->execute();
$rh_results = $sth->fetchall_hashref('tabid');

$$rrh_results = $rh_results;

^
^

You could remove that dollar sign.

But then you don't need $rh_results at all:

$rrh_results = $sth->fetchall_hashref('tabid');

return(0);
}

##------------------------------------------------------------------
my $conn = undef;
my %results = ();

fetchall_hashref() allocates the hash and returns a reference
to it, you do not need %results at all.

if(Connect('sysmaster', \$conn) < 0)
{
exit(-1);
}
if(GetTableValues($conn, \\%results) < 0)

my $results;
if(GetTableValues($conn, $results) < 0)

{
$conn->disconnect;exit(-1);
}
#print Dumper %results; - No output produced by this print
if(ProcessTableValues(\%results) < 0)

if(ProcessTableValues($results) < 0)

{
$conn->disconnect;exit(-1);
}
$conn->disconnect;
exit(0);

If you put your disconnect() call in an END block, then you wouldn't
have to sprinkle them all around your code.

 

[niall.macpherson]

If you put your disconnect() call in an END block, then you wouldn't
have to sprinkle them all around your code.


--

Uri Guttman wrote

Thanks Uri, Tad and Paul for all the very useful advice.

I must admit the only time I have used the END block has been when I
have followed it with data for testing using while<DATA> . In my real
program (which is a couple of thousand lines long) , it could be made
far more readable by using this. Don't know why I didn't realise it
before

Uri's solution of storing the hash ref as an element of the return ref
looks to be the best method without having to modify $_[1] (which is
very neat but might confuse others without copious documentation) and
without doing a deep copy (which I wish to avoid if at all possible).

I will give these a try !

 

[Mons]

Agree with previous posters buw want to add a general notices...
1. Ok, it's kinda standart in C to return 0 on succes and -1 on
failure.
But as noticed Paul Lalli, we're programming Perl
so

if(ProcessTableValues(\%results) < 0)
{
$conn->disconnect;exit(-1);
}

will look nicer such way:

unless (ProcessTableValues(...)) {
$conn->disconnect;
exit(-1);
}

(In case, that on failure it returns 0, and 1 or something on success
At all, I think it's logical mistake to return `false' in case of
success)

2.

print STDERR "\n", 'Failed to prepare ' , $DBI::errstr;

Be simplier!
warn 'Failed to prepare ' . $DBI::errstr;

3. Surely, you don't need to do separate prepare,execute and fetch.
Call once
$conn->selectall_hashref('select tabid, tabname from systables where
tabid < 5','tabid')

4. And finally
For such a routines may be better to write them in this way:

sub some_func ($$) {
my ($param1,$param2) = @_;
eval {
# do here all stuff without checking at every step
stuff1();
stuff2() or warn "noncritical IO stuff failed: $!";
stuff2() or die "critical stuff failed";
};
if ($@){ # if in eval block was catched die
warn $@;
return 0; #failure
}else{
return 1; #success
}
}

With this guides, your program may be rewritten in about 20 lines.
While shorter code is more readable and powerful

PS: Sorry for bad English, if somewhere

 

[A. Sinan Unur]

Agree with previous posters buw want to add a general notices...
1. Ok, it's kinda standart in C to return 0 on succes and -1

-1 ???

(In case, that on failure it returns 0, and 1 or something on success
At all, I think it's logical mistake to return `false' in case of
success)

I can't parse this.

3. Surely, you don't need to do separate prepare,execute and fetch.

Well, that separation is actually a good idea in general. You prepare
once, execute when needed, and fetch while you want more results.

4. And finally
For such a routines may be better to write them in this way:

sub some_func ($$) {

Why the prototype?

my ($param1,$param2) = @_;
eval {
# do here all stuff without checking at every step
stuff1();
stuff2() or warn "noncritical IO stuff failed: $!";
stuff2() or die "critical stuff failed";

So, if stuff2() returns false, both a noncritical and a critical failure
occurred?

Sinan

 

[xhoster]

I have a sub which calls the fetchall_hashref method from the DBI
module. I like to have all my subs return either 0 (success) or -1
(failure) and if the sub need to return some data to the caller I
always pass a variable by reference and populate that variable . I tend
to try to think of refs the same way as I did with pointers in 'C'
which I am more familiar with. What I am trying to do here is pass a
reference to a reference to a hash to the sub.

As others have said, you should not program C in Perl. Program Perl in
Perl.

my %attr = ( RaiseError => 1, AutoCommit => 0);
$$r_conn = DBI->connect("dbi:ODBC:" . $dsn , undef, undef,
\%attr);

You said you want your subroutines to return -1 on failure. However,
because you have RaiseError => 1, they are generally not going to return
anything on failure. Instead, DBI will raise errors, i.e. call die. You
could intercept that with eval{}, but in the code you show doesn't do that.

Xho

 

[Uri Guttman]

M> Agree with previous posters buw want to add a general notices...
M> 1. Ok, it's kinda standart in C to return 0 on succes and -1 on
M> failure.
M> But as noticed Paul Lalli, we're programming Perl
M> soM> will look nicer such way:

M> unless (ProcessTableValues(...)) {
M> $conn->disconnect;
M> exit(-1);
M> }

that is better perl style i agree but the OP has his return value style
so ingrained i didn't want to comment on it.

M> 2.M> Be simplier!
M> warn 'Failed to prepare ' . $DBI::errstr;

be simpler:

warn "Failed to prepare $DBI::errstr" ;

M> 3. Surely, you don't need to do separate prepare,execute and fetch.
M> Call once
M> $conn->selectall_hashref('select tabid, tabname from systables where
M> tabid < 5','tabid')

dbi prepare is useful, especially if you use placeholders. on some db
servers it can actually be much faster since the sql is only compiled
once.

M> 4. And finally
M> For such a routines may be better to write them in this way:

M> sub some_func ($$) {

prototypes are not cool in perl in most cases.

M> my ($param1,$param2) = @_;
M> eval {
M> # do here all stuff without checking at every step
M> stuff1();
M> stuff2() or warn "noncritical IO stuff failed: $!";
M> stuff2() or die "critical stuff failed";
M> };

but you are checking the last two steps. using eval BLOCK to catch
errors is ok when you have deep nesting and you aren't sure where the
error should be handled. this style is also called exception handling
and there are modules to make it easier to do. but a clean return value
design with proper handling can do the job just as effectively.

M> With this guides, your program may be rewritten in about 20 lines.
M> While shorter code is more readable and powerful

that is true to a point. code written for the sake of just shortness can
be harder to maintain and debug. there is a balance that must be found
between concise vs. bloated

uri

 

[Tad McClellan]

I must admit the only time I have used the END block has been when I
have followed it with data for testing using while<DATA> .

An END block:

END { $conn->disconnect; }

has nothing to do with the __END__ token nor with the input operator.

See:

perldoc perlmod

 

[niall.macpherson]

Thanks for all the various replies - I must get myself thinking in Perl
rather than C / C++.

The reason for the minimal error handling was because I was trying to
post a small self contained program that would run stand alone

This was also the reason for $conn->disconnect;exit(-1); all
appearing on one line so that the whole program could be seen in as
small a space as possible.

Re the error handling - the actual program currently contains about
900 - 1000 lines (at the moment) and uses Tk. The reason for a failure
will normally be because the user has entered some invalid information
in which case I normally pop up a message box and continue .

I cannot really think of any circs I will want to exit other than the
user closing the top level window - even if the database engine is
down I will probably just pop up an error message and the user can (if
required) sort out the db issues and retry.

Should I be looking at Carp ? I have never used this before but seeing
as the utility I am writing is now getting quite large. I would like in
particular to dump a stack trace when I encounter an error since during
development tracking down exactly where the sub which caused the error
was called from can be quite time consuming. As far I can see from the
CPAN docs Carp will allow me to do this.

 

[niall.macpherson]

A. Sinan Unur wrote:

Well, that separation is actually a good idea in general. You prepare
once, execute when needed, and fetch while you want more results.

Yes - the reason I prepare separately is because I have certain SQL
commands which need to be executed many times with varied input.
Preparing the statement once allows me to catch any syntax errors at
the start and also improves performance.