I'd do a function. Maybe tomorrow you will need the same with another small variation.

sub foo {
  my ($table, $hr) = @_;
  ## TODO: argument checking
  $Data->Sql ("SELECT * FROM $table");
  while ($Data->FetchRow ()) {
    my %x= $Data->DataHash();
    $hr->{$x{ID}}{$x{DataName}}= $x{DataValue};
  }
}

foo "ConfigData", \%ConfigData;
foo "DicomDestinations", \%DestinationData;
[download]

I saw what some other people have done and to a certain degree they are missing the point of encapsulation and design.

1. You want a subroutine that prevents redundancy, excellent
2. There is only one variable changed in the request
3. There is only one variable changed from the result
4. Keep your code focused, make it simple and do not do too many things at one time

sub get_rows_from {
  my $dbh   = shift;
  my $table = shift;

  $dbh->Sql ("SELECT * FROM " . $table);

  my %result;
  while ($dbh->FetchRow() ) {
    my %x= $dbh->DataHash();
    $result{ $x{ID} } = \%x;
  }
  return \%result;
}

my $config_data_ref = $dbh->get_rows_from( 'ConfigData' );
my $dicom_data_ref  = $dbh->get_rows_from( 'DicomDestinations' );
[download]

So what we have a subroutine, with a simple interface. We only have to pass one thing to it (the table name). We will get back a hash reference. Since we are not doing data manipulation in the subroutine, we do not have to guess what the result will be if we passed a hash reference to the subroutine.

I think you made a good point: one of the changed things is a parameter, but the other is a result and should be handled differently.

I'd thought about making it a function or a loop body with the other concepts mentioned on this thread: pairs, or a hash mapping the name to the variable. That is enlightening: thanks.

—John

I'll take a stab at it,

my %Control = (
    ConfigData => \%ConfigData,
    DicomDestinations => \%DestinationData
);

for my $set (qw/ConfigData DicomDestinations/) {
    $Data->Sql ("SELECT * FROM $set");
    while ($Data->FetchRow ()) {
        my %x = $Data->DataHash();
        $Control{$set}{$x{ID}}{$x{DataName}}= $x{DataValue};
    }
}
[download]

Is it worth it? That depends on how likely it is that the two operations will diverge in the future.

Added: Another way, probably better,

sub DataClass::doit {
    my ($self, $set, $hash) = @_;
    $self->Sql ("SELECT * FROM $set");
    while ($self->FetchRow ()) {
        my %x = $self->DataHash();
        $hash->{$x{ID}}{$x{DataName}}= $x{DataValue};
    }
    1;
}

$Data->doit('ConfigData',\%ConfigData);
$Data->doit('DicomDestinations', \%DestinationData);
[download]

-- Hugh

It is certainly possible to golf this down, and it is an interesting exercise, but in practice it's unlikely to make the code more legible or reusable, so for production purposes I would not recommend it.

As far as how to golf it down, my first thought would be to reach for map update: like the following...

map{$Data->Sql("SELECT * FROM $_");
while($x=$Data->fetchrow_hashref){
$ConfigData{$$x{ID}}{$$x{DataName}}=$$x{DataValue}};
}qw(ConfigData DicomDestinations);
[download]

I'm sure someone can shave some more strokes off that.

The map doesn't buy you much in terms of golf here, and he's also changing the target hash. I'd probably approach it something more like:

my @table_to_hash = (
    [ "ConfigData",        \%ConfigData      ],
    [ "DicomDestinations", \%DestinationData ]
);

for (@table_to_hash) {
    my ($table, $hash) = @$_;

    $Data->Sql("SELECT * FROM $table");

    while ($Data->FetchRow ()) {
        my %x = $Data->DataHash();
        $hash->{ $x{ID} }{ $x{DataName} } = $x{DataValue};
    }
}
[download]

Update: I see this is almost the same as Zaxo's post at Re: reuse worth it for small items?, but note that mine will maintain the order. I'm not sure it matters, but it could.