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

I've written a simple cgi script that generates the company phone list from a sql backend. I'm looking for some advice on simplifing this branch structure. There is a strong pattern here and I can think of a few simple ways to reduce the code but I wanted to see what other people would do to make this more managable. 
if ($type eq 'f') {     
        if ($ref->{'pic'}) {
                print $q->Tr([
                        $q->td({-bgcolor=>$cellcolor},[$q->a({href=>"$
+ref->{'pic'}"},$ref->{'name'}),$ref->{'phone'},$ref->{'fax'},$ref->{'
+loc'},
                        $q->a({href=>"mailto:$ref->{'email'}"},$ref->{
+'email'}) ])
                ]);
        } else {
                print $q->Tr([
                        $q->td({-bgcolor=>$cellcolor},["<LI>".$ref->{'
+name'},$ref->{'phone'},$ref->{'fax'},$ref->{'loc'},
                        $q->a({href=>"mailto:$ref->{'email'}"},$ref->{
+'email'}) ])
                ]);
        }
} elsif ($type eq 'c') {                
        if ($ref->{'pic'}) {
                print $q->Tr([
                        $q->td({-bgcolor=>$cellcolor},[$q->a({href=>"$
+ref->{'pic'}"},$ref->{'name'}),$ref->{'phone'},$ref->{'fax'},$ref->{'
+loc'},
                        $ref->{'con'}])
                ]);
        } else {
                print $q->Tr([
                        $q->td({-bgcolor=>$cellcolor},["<LI>".$ref->{'
+name'},$ref->{'phone'},$ref->{'fax'},$ref->{'loc'},
                        $ref->{'con'}])
                ]);
        }
} else {
        if ($ref->{'pic'}) {
                print $q->Tr([
                        $q->td({-bgcolor=>$cellcolor},["<IMG SRC=\"/im
+ages/thumb_$ref->{'pic'}\">&nbsp &nbsp".$q->a({href=>"/images/$ref->{
+'pic'}"},$ref->{'name'}),$ref->{'phone'},$ref->{'cell'},$ref->{'loc'}
+,
                        $q->a({href=>"mailto:$ref->{'email'}"},$ref->{
+'email'}) ])
                ]);
        } else {
                print $q->Tr([
                        $q->td({-bgcolor=>$cellcolor},['<LI>&nbsp &nbs
+p '.$ref->{'name'},$ref->{'phone'},$ref->{'cell'},$ref->{'loc'},
                        $q->a({href=>"mailto:$ref->{'email'}"},$ref->{
+'email'}) ])
                ]);
        }
}

[download]

Replies are listed 'Best First'.
Re: Simplifying repetitive flow structures
by gav^ (Curate) on Apr 01, 2002 at 02:15 UTC
    Ick. That's really hard to read through. Everything pretty much looks the same. I also find that using CGI's methods to generate HTML to create messy code and prefer to use a templating module such as HTML::Template. A template something like this should do the trick: 
    <TMPL_LOOP PHONEBOOK>
    <tr>
    <td>
        <TMPL_IF PIC>
            <TMPL_IF TYPE_OTHER>
                <img src="/images/thumb_<TMPL_VAR NAME=PIC>"
            </TMPL_IF>
                <a href="<TMPL_VAR NAME=PIC"><TMPL_VAR NAME=NAME></a>
       <TMPL_ELSE>
            <TMPL_VAR NAME=NAME>
        </TMPL_IF>
    </td>
    <td><TMPL_VAR NAME=PHONE></td>
    <td><TMPL_VAR NAME=FAX></td>
    <td><TMPL_VAR NAME=LOC></td>
    <td>
        <TMPL_IF TYPE_C>
            <TMPL_VAR NAME=CON>        
        <TMPL_ELSE>
            <a href="mailto:<TMPL_VAR NAME=EMAIL>"><TMPL_VAR NAME=EMAI
+L></a>
        </TMPL_IF>
    </td>    
    </tr>
</TMPL_LOOP>

    [download]
    Then all you have to do is create an LoH (list of hashes) that has your data plus TYPE_C and TYPE_OTHER set and use it like: 
    my $t = HTML::Template->new;
$t->param(PHONEBOOK => \@data);
print $t->output;

    [download]
    This has the advantage of seperating out your code and design and allowing somebody else to tweak the output. For more info on HTML::Template see its documentation or using CGI, DBI and HTML::Template (a mini tutorial with example code).

    Hope this helps.

    gav^

[reply]
[d/l]
[select]
      That code does look pretty terrible when it's line wrapped. I'm not using HTML::Template for alot of reasons - mostly because this is running under mason/mod_perl and I'm trying to limit the number of differnt things to maintain. Code/html seperation with Mason is something I've considered. However your example doesn't simplify the problem. I still have to deal with variable numbers of columns with differing names depending on the value of "$type" and in addition to that the person may or maynot have an image in the directory as well (if 'pic' is defined or not).
[reply]
        HTML::Template works great under mod_perl. Look in the FAQ section of the docs and set cache/shared_cache in the call to HTML::Template->new. If HTML::Template doesn't cut it then Template Toolkit (or here) is more powerful has support for expressions amongst things. Code/HTML seperation for me is not just a "good thing" it is a business critical thing. Generating HTML with CGI.pm just doesn't cut it in 2002.

        I gave a answer for the question you asked. I can only work with the information you gave, and I thought I covered all the bases. Maybe you need to re-think the design? Poor implementations are normally a symptom of bad design.

        gav^

[reply]
Re: Simplifying repetitive flow structures
by eisenstein (Acolyte) on Apr 01, 2002 at 08:11 UTC
    I think the problem is that you're trying to build the Tr all at once. Use the conditionals to determine your branching, and leave the stuff that's not conditional (e.g. $ref->{'loc'}) outside of the if blocks. Construct the Tr after you've figured out what it's made of.

    The below code doesn't exactly reproduce the functionality of your code, but it's probably good enough (or can be adjusted). 

    my @tds = ();
if ($ref->{'pic'}) {
    push @tds, $q->a({href=>"$ref->{'pic'}"},$ref->{'name'});
} else {
    push @tds, "<LI>".$ref->{'name'};
}

push @tds, $ref->{'phone'};

if ($type eq 'f' || $type eq 'c') {
    push @tds, $ref->{'fax'};
} else {
    push @tds, $ref->{'cell'};
}

push @tds, $ref->{'loc'};

if ($type eq 'c') {
    push @tds, $ref->{'con'};
} else {
    push @tds, $q->a({href=>"mailto:$ref->{'email'}"},$ref->{'email'})
+;
}

print $q->Tr([$q->td({-bgcolor=>$cellcolor},\@tds)]);

    [download]
[reply]
[d/l]
      I think is a much better and maintainable way of doing things. I can even adjust the table headers by evaluating the array in a scalar context. Great Idea, Thanks!
[reply]
Re: Simplifying repetitive flow structures
by admiralh (Sexton) on Apr 01, 2002 at 16:55 UTC
    The main thing you need to do here is abstract out all the commonalities. From my eyeballing, it seems that the only thing changing is the contents of the Tr data.

    Try this 

    
my $td_ref;
if ($type eq 'f') {     
  if ($ref->{'pic'}) {
    $td_ref = [$q->a({href=>"$ref->{'pic'}"}, $ref->{'name'}),
               $ref->{'phone'}, $ref->{'fax'}, $ref->{'loc'},
               $q->a({href=>"mailto:$ref->{'email'}"}, $ref->{'email'}
+)
              ];
  } else {
    $td_ref = ["<LI>".$ref->{'name'},
               $ref->{'phone'}, $ref->{'fax'}, $ref->{'loc'},
               $q->a({href=>"mailto:$ref->{'email'}"},$ref->{'email'})
              ];
  }
} elsif ($type eq 'c') {                

# and so on ...

}
print $q->Tr([$q->td({-bgcolor=>$cellcolor},$td_ref)]);

    [download]
    I also added a bit of spacing for readibility.
[reply]
[d/l]

Back to Seekers of Perl Wisdom