http://www.perlmonks.org?node_id=469269

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

Hello good monks.

I've run into a problem with my best code and project so far.

It's not necessarily erroneous code, oh no, it's bigger than that. It's my code elegance and code efficiency that I'm having troubles with.

By looking at my code, one should be able to clearly see my objective: retrieve the necessary data from their respective tables, and package them up all nice for printing to the browser.

Wonderful, dandy, works, and everyone is happy. Yea, except for the fact that I copy and pasted the same block of code n times for each piece of data I wanted.

Can a brutha get some help?

Thanks in advance,

dhoss



In essence, I'd like to be able to compact all these here for loops into one grab of the data. I can't think of a way to do this, of course, so I've come to you, the wise monks of the Monastery


bows gratefully

#!perl -w use strict; use PL; my $end; my $start ; my $obj = PL->new; my $q = $obj->CGI; $obj->Template->file ("tmpl/main.tmpl"); my @r_data = (); my @loop_data = (); my @c_data = (); my @ids = $obj->DBI->Quote->sql_ids; #my $quote = $obj->DBI->Quote->retrieve(rand @ids ); my @info = $obj->DBI->Entries->retrieve_from_sql(qq{ hidden = 'n' ORDER BY date desc LIMIT 1 }); my @comments = $obj->DBI->Replies->retrieve_from_sql(qq{ hidden = 'n' ORDER BY date desc LIMIT 5 }); my @recent = $obj->DBI->Entries->recent; print $q->header; for (@comments) { my %data; $data{c_title} = $_->title; $data{author} = $_->author; $data{id} = $_->thread_id; $data{c_id} = $_->id; push @c_data, \%data; } for (@info) { my %data; my $sth = $obj->DBI->Replies->sql_count; $sth->execute($_->id); my $r = ($sth->fetchrow_array)[0]; $data{author} = $_->author; $data{content} = $_->content; $data{title} = $_->title; $data{id} = $_->id; $data{date} = $_->date; $data{count} = $r; push @loop_data, \%data; } for (@recent) { my %data; $data{author} = $_->author; $data{title} = $_->title; $data{id} = $_->id; $data{date} = $_->date; push @r_data, \%data; } if ( $q->param('view') =~ m/last5/ ) { ## Kinda redundant...but oh well, I'll update it later. my @loop_data = (); my @info = $obj->DBI->Entries->retrieve_from_sql(qq{ hidden = 'n' ORDER BY date desc LIMIT 5 }); for (@info) { my %data; my $sth = $obj->DBI->Replies->sql_count; $sth->execute($_->id); my $r = ($sth->fetchrow_array)[0]; $data{author} = $_->author; $data{content} = $_->content; $data{title} = $_->title; $data{id} = $_->id; $data{date} = $_->date; $data{count} = $r; push @loop_data, \%data; } print $obj->Template->format ( { title=>"Devin's Journal", body=> \@loop_data, recent_c => \@c_data } ); } else { print $obj->Template->format ( { title=>"Devin's Journal", body=> \@loop_data, recent_c => \@c_data, show => $q->param ? 1 : 0 } ); }
meh.

Replies are listed 'Best First'.
Re: My excessive and redundant code<333
by graff (Chancellor) on Jun 23, 2005 at 03:00 UTC
    My my my. Perhaps someone here is being compulsively hard to please? Compulsively self-critical? Or maybe just a compulsive code reducer who doesn't quite understand yet what is meant by the ancient chant: "If it ain't broke, don't fix it"...

    I don't see anything wrong with that code. The "repetitive" for loops make it clear that three distinct data sources are being used, and that these each have distinct amounts and types of information (as well as some common attributes), and yet each loop is compact enough to make it clear that the same general method of treatment is needed for each source (with the potential for minor variations per source). This strikes me as an optimal balance in the trade-off between what folks used to call "in-line vs. structured" coding.

    To condense it any further would risk tipping the balance in the direction of obfuscation and more difficult maintenance. For example, you could try to "objectify" it in some way, but your current use of objects is already hiding a lot of hard work and a lot of design details -- a lot is left to the imagination for the "uninitiated" code reader (that is, the reader realizes that other sources need to be consulted in order to see what's really happening here). If you abstract further (and especially if you forget to add essential commentary/pod), everyone except you will be more likely to look at it and just say "Huh? What's this doing?"

    The same goes for trying to use some sort of "uber" data structure (HoAo... or AoHo...) to shove the three different things into a single loop over that structure. I wouldn't do that in this sort of case, because the data structure won't be any easier to understand or maintain than the various for loops.

    Sorry, I guess I'm not "helping" in the way you would like... maybe I'm just too old-fashioned.

      Maybe you're right graff. I had just seen some code very similar to mine that had somehow compacted their calls for loading up the data into the needed array and using it.

      Thanks for the insight...sometimes one needs to just step back and realize IT WORKS...leave it be.
      ++ for sure :-)
      meh.
Re: My excessive and redundant code<333
by bobf (Monsignor) on Jun 23, 2005 at 04:07 UTC

    If it works and it is readable, I wouldn't be too concerned about a few for loops. That said, I'd probably do two things: use hash slices (see perldata) instead of all of the individual $data{key} = $_->key; statements, and pull the guts of the for ( @info ) loop into a subroutine (since you use it twice). For example (untested):

    for ( @info ) { push( @loop_data, get_info( $_ ) ); } sub get_info { my ( $info ) = @_; my $sth = $obj->DBI->Replies->sql_count; $sth->execute( $info->id ); my $info->count = ( $sth->fetchrow_array )[0]; my @keys = qw( author content title id date count ); my %data; @data{ @keys } = @$info{ @keys }; return \%data; }

    Note: I changed $r to $info->count to clean up the hash slice. Update: Corrected $count to $info->count.

    HTH, and congrats on getting it working. That's the first step. :-)

Re: My excessive and redundant code<333
by moot (Chaplain) on Jun 23, 2005 at 02:41 UTC
    Would Class::DBI help?

    --
    Now hiring in Atlanta. /msg moot for details.

      I'm using Class::DBI. I just don't know how to skim it down to one call of retrieve or whatever method need be called.
      meh.

        First thing to mind is how you do the same things over and over - and I'm not sure the first thing to my mind is what's coming to your mind. ;-)

        A quick perusal of Class::DBI gives me this thought. First off, it's way easier if you can keep the names the same ... so here we try:

        my @comment_cols = qw/title author thread_id id/; for (@comments) { my %data; @data{@comment_cols} = $_->get(@comment_cols); push @c_data, \%data; }
        Or, if you can't keep them the same:
        my %comment_cols = qw/ c_title title author author id thread_id c_id id /; for (@comments) { my %data; @data{keys %comment_cols} = $_->get(values %comment_cols); push @c_data, \%data; }
        Then push the definition (@comment_cols or %comment_cols) out into a package variable so that it only is created once, and then adding/removing/renaming columns becomes a bit easier. After all that, I also notice that anything that says "for ... push" looks like a "map" to me. So I get:
        @c_data = map { my %data; @data{keys %comment_cols} = $_->get(values %comment_cols); \%data; } @comments;
        Then you can take this loop or map, put it into a subroutine that takes the column list or column map as a reference, takes the input from Class::DBI as another reference, and returns the transformed data as a list of hashes. And then you'll be a long way towards making this easier to deal with, IMO. Although I have to admit, it's not really that bad now. I would just anticipate some column renamings by these steps, just to make things easier on me when I change my mind about the name of a column or something. :-)

Re: My excessive and redundant code<333
by trammell (Priest) on Jun 23, 2005 at 03:16 UTC
    After skimming the code, I'm pretty sure it's some sort of CGI script. A few lines of documentation wouldn't hurt.

    I also notice that you declare $end at the top of the script, then never use it. I suspect there are other problems, but I usually stop when I see a declared variable that isn't used.

      It is indeed a CGI script and apologies for lack of documentation. I need to take those $start and $end vars out, those are remnants of an old method of timing the page generation.
      meh.