PERL Beginners - Returning multiple MySQL rows from module

Author

Returning multiple MySQL rows from module

Steve Bertrand

2007-11-27, 10:01 pm

Hi all,

I am trying to modularize some standard MySQL queries that I frequently
do against our RADIUS DB.

The current setup feels very 'kludgy' to me. The code snip below comes
out of the module. I hope there is enough context to see why I think it
can be improved.

Essentially, I'd like a graceful way for the module to perform the
query, and then as neatly as possible pass back the result in a manner
that would require the minimal amount of processing when it's returned
back to the initial application. In almost all cases, there will be
multiple rows returned:

--- snip ---

$query->execute;

my @current_row;
my @total_rows;

while (@current_row = $query->fetchrow_array()) {
push (@total_rows, [@current_row]);
@current_row = ();
}

return (@total_rows);

--- end snip ---

Regards,

Steve

Tom Phoenix

2007-11-27, 10:01 pm

On 11/27/07, Steve Bertrand <iaccounts@ibctech.ca> wrote:

> The current setup feels very 'kludgy' to me. The code snip below comes
> out of the module. I hope there is enough context to see why I think it
> can be improved.

Actually, the snippet you submitted seems pretty straightforward. It
could be improved (what code couldn't?) but it's not in *need* of
improvement, IMHO: It can't become impressively faster or clearer.
What seems kludgy about it? What improvement are you s

ing?

--Tom Phoenix
Stonehenge Perl Training

Steve Bertrand

2007-11-27, 10:01 pm

Tom Phoenix wrote:
> On 11/27/07, Steve Bertrand <iaccounts@ibctech.ca> wrote:
>
>
> Actually, the snippet you submitted seems pretty straightforward. It
> could be improved (what code couldn't?) but it's not in *need* of
> improvement, IMHO: It can't become impressively faster or clearer.
> What seems kludgy about it? What improvement are you s

ing?

Well, I didn't like the fact I was returning an AoA. I have subsequently
changed the code to only allow a single row return, and assign it
directly to $self via a hashref instead:

-- snip --

while (my $service_info = $query->fetchrow_hashref) {

for my $field (keys %$service_info) {
$self->{$field} = $service_info->{$field};
}
}

-- end snip --

Thanks for your response Tom!

Steve

Rob Dixon

2007-11-27, 10:01 pm

Steve Bertrand wrote:
>
> Tom Phoenix wrote:
>
> Well, I didn't like the fact I was returning an AoA. I have subsequently
> changed the code to only allow a single row return, and assign it
> directly to $self via a hashref instead:
>
>
> -- snip --
>
> while (my $service_info = $query->fetchrow_hashref) {
>
> for my $field (keys %$service_info) {
> $self->{$field} = $service_info->{$field};
> }
> }
>
> -- end snip --
>
> Thanks for your response Tom!

Hi Steve.

This doesn't look right at all. Your original code was

Steve Bertrand wrote:
>
> $query->execute;
>
> my @current_row;
> my @total_rows;
>
> while (@current_row = $query->fetchrow_array()) {
> push (@total_rows, [@current_row]);
> @current_row = ();
> }
>
> return (@total_rows);

which returns a list of database records. Fine. Nothing wrong with
returning a list or arrays if that's the shape your data is.

In your new code you retrieve each record as a hash, keyed by field
name. Then, instead of your subroutine returning the result as it did
before, you copy the hash elements directly into your object.

Getting DBI to return a hash instead of an array is quite a lot slower,
although that may not matter. But more worryingly you are overwriting
the object values each time you fetch a record, so that when the loop
exits only the last set of data is retained. Is that what you want? If
you are expecting only a single record to be returned then you should
only do a single fetch. But it is hard to see the code you have written
taking the place of what went before, which just collected all the data
records and returned them.

I would always be hesitant to alter existing code if it works and is
readable. Only ever change it if it is too slow or incomprehensible.

Rob

Steve Bertrand

2007-11-28, 7:01 pm

> In your new code you retrieve each record as a hash, keyed by field
> name. Then, instead of your subroutine returning the result as it did
> before, you copy the hash elements directly into your object.

Correct. Essentially what I want is to absolutely minimize any work I
have to do outside of the module. This particular module subroutine will
need to be called from several different applications, so instead of
returning an AoA, then having to process through that in each
application, the more data directly assigned via the subroutine, the
less work I have to do later.

> Getting DBI to return a hash instead of an array is quite a lot slower,
> although that may not matter.

In this case, it does not.

> But more worryingly you are overwriting
> the object values each time you fetch a record, so that when the loop
> exits only the last set of data is retained. Is that what you want?

Well, not exactly. I will see either 0, one or two records per call.
It's looking for a user's RADIUS month history, and I will have either
no records, a record for DSL usage, dialup usage or both. See below what
I have done to see if it makes better sense now, or if it can be made
better.

> I would always be hesitant to alter existing code if it works and is
> readable. Only ever change it if it is too slow or incomprehensible.

I agree, however, this is all in the name of minimizing code writing
later (if this makes sense). Here is the entire sub. Hopefully It won't
wrap uncontrollably:

--- snip ---

my $self = shift();
my $dbh = shift();
my $username = shift();
my $date = shift();
my $hs_record = 0; # HighSpeed row
my $du_record = 0; # DialUp row

my $query = $dbh->prepare("

SELECT * FROM mtotacct
WHERE UserName = '$username'
AND AcctDate LIKE '$date%';");

$query->execute() or die $dbh->errstr;

# Extract the info from the DB, and assign it to $self

my $hs = 'highspeed';
my $du = 'dialup';

while (my $service_info = $query->fetchrow_hashref) {

for my $field (keys %$service_info) {

if ($service_info->{'NASIPAddress'} =~ /208.70/) {
$self->{$du}->{$field} = $service_info->{$field};
$du_record = 1;

} elsif ($service_info->{'NASIPAddress'} =~ /199.166/) {
$self->{$hs}->{$field} = $service_info->{$field};
$hs_record = 1;
}
}
}

# Return back either a true or false for dialup and highspeed
# I'm wondering at this point if I should just go back to the
# returning of the AoA, as I'm in a situation where I now am
# processing an array again anyhow.

my @records = ($du_record, $hs_record);
return (@records);

--- end snip ---

Thanks,

Steve

Sponsored Links