in reply to HTML::Template nested loops, DBI/MySQL and map

I think it looks pretty good. Your use of DBI, its shortcut methods (selectall_arrayref and fetchall_arrayref), and the Slice options is spot-on: this is exactly why these methods are there.

I only have some stylistic comments, prompted by the fact that add_subject_loop modifies one of its arguments.

  1. I would try to remove the side-effects, and make it a proper function
  2. I would try to move the sql strings and the preparation of the subject handle out of get_category_loop
I believe that would improve the structure of your program, and increase its maintainability. Abstracting away the lower-level sql stuff is a good idea. See for instance Perl Hacks for some pointers.

I'm also a big fan of prepare_cached, since it can remove the need of global $sth variables.

Here's a suggested refactoring:

sub get_category_loop { return [ map { $_->{subject_loop} = get_subject_loop( delete $_->{category_id} ); # dele +te returns the value that was in the hash $_ } @{ $dbh->selectall_arrayref(sql_category(), {Slice => {}}) } ]; } sub get_subject_loop { my $category_id = shift; my $sth = $dbh->prepare_cached( sql_subject() ); $sth->execute($category_id); return $sth->fetchall_arrayref({}); } sub sql_category { return q{ SELECT category_id, category_name FROM category ORDER BY category_id }; } sub sql_subject { return q{ SELECT subject_id, subject_name FROM subject WHERE category_id = ? ORDER BY subject_name }; }
I'm not so sure anymore if the map is justified. I feel a foreach loop would probably be easier to read now.

Replies are listed 'Best First'.
Re^2: HTML::Template nested loops, DBI/MySQL and map
by wfsp (Abbot) on Nov 14, 2006 at 14:26 UTC
    Many thanks for your comments and suggestions. I have taken them all on board.

    I was going to ask about passing the statement handle around - it didn't feel right. Neatly solved.

    I also agree that a for loop is now easier to read than the map. Again, many thanks.

    sub get_category_loop { my @cat_loop_hrefs = @{ $dbh->selectall_arrayref( sql_category(), {Slice => {}} ) }; for my $cat_href (@cat_loop_hrefs){ $cat_href->{subject_loop} = get_subject_loop(delete $cat_href->{category_id}); } return [@cat_loop_hrefs]; }