in reply to Tk programming style

In quite a few locations you could have abstracted a bit more. F.ex: 
my $cmds = $tl->Frame;
$cmds->pack;
    
if (defined $idx) {
# Existing activity - OK Apply Delete Cancel
    $cmds->Button(-text => 'OK', -command => sub {&edit_entry($idx,$le
+gentry);$tl->destroy;})
         ->pack(-side => 'left');
    $cmds->Button(-text => 'Apply', -command => [\&edit_entry, $idx, $
+legentry])
         ->pack(-side => 'left');
    $cmds->Button(-text => 'Delete', -command => sub {&delete_entry($i
+dx);$tl->destroy;})
            ->pack(-side => 'left');
} else {
# New activity - OK Apply Cancel
    $cmds->Button(-text => 'OK', -command => sub {&add_entry($legentry
+);$tl->destroy;})
         ->pack(-side => 'left');
    $cmds->Button(-text => 'Apply', -command => [\&add_entry, $legentr
+y])
         ->pack(-side => 'left');
}
$cmds->Button(-text => 'Cancel', -command => [$tl => 'destroy'])->pack
+(-side => 'left');

[download]
You can write that as: 
my @button = defined $idx ? (
    OK     => sub { &edit_entry($idx,$legentry); $tl->destroy; },
    Apply  => [\&edit_entry, $idx, $legentry],
    Delete => sub { &delete_entry($idx); $tl->destroy; },
    Cancel => [$tl => 'destroy'],
)
: (
    OK     => sub { &add_entry($legentry); $tl->destroy; },
    Apply  => [\&add_entry, $legentry],
    Cancel => [$tl => 'destroy'],
);

for($tl->Frame) {
    $_->pack;
    while(my ($text, $command) = splice @button, 0, 2) {
        $_->Button(-text => $text, -command => $command)
          ->pack(-side => 'left');
    }
}

[download]

Note how the layout now directly conveys the information you previously had to sum up in comments.

If you don't like the scoping to main:: (I don't either), and you have a relatively involved data structure, that usually means you should abstract out your backend logic into a class. Use Class::Accessor to save the hassle of writing a lot of accessors and mutators and life is good. See also Damian Conway's ten rules for when to use OO.

F.ex given a class Activity, and with a slightly different INI format, you could have wittled down this:

print SAVE <<END;
for (0..$#activity) {
    my ($legend,$time,$start,$end,$elapsed) = 
        @{%{$activity[$_]}}{qw(legend timstr start end elapsed)};

# Add up times of current activity
    my $tsecs = 0;
    for my $i (0..$#$end) {
        $tsecs += tv_interval($start->[$i],$end->[$i]);
    }
    $tsecs += $elapsed if $elapsed;
    $tsecs += tv_interval($start->[-1],[gettimeofday]) if defined($cur
+rent) && ($_ == $current);
    print SAVE <<END2;
[Button $legend]
time=$time
elapsed=$tsecs
END2
}

[download]
to this: 
for (@activity) {
    print SAVE "[Button]\n";
    my $item;
    print SAVE "$item=" . $_->$item() for qw(legend time elapsed);
}

[download]

with calculation for time elapsed being hidden away at the particular spot, since it's not where it logically belongs anyway.

In general, try to abstract a bit more - capture regularities in code, irregularities in data.

Makeshifts last the longest.

Replies are listed 'Best First'.
Re: Re: Tk programming style
by rinceWind (Monsignor) on Nov 07, 2002 at 13:15 UTC
    Thank you Aristotle, this is exactly the kind of feedback I was looking for.

    Your idea for handling the buttons in the activity_configure sub is excellent - very clear and concise. Though I would probably have used a hash instead of @button, and used each instead of splice.

    Regarding OO, this has prompted me to review and assess the plethora of Class::* modules on CPAN. Having activity as a class will present a neater way of dealing with the callbacks, as methods, but it still looks that "Next" will be difficult to implement. Can I get the toplevel widget to return me the next frame I wonder?

    Regarding the .ini save, I prefer my original, as the heredoc stands out and makes it obvious what is happening. Admittedly, I could refactor the time calculation into a sub - which may become a method in my new class.

[reply]
[d/l]
[select]
      But the way the save loop is set up, it reduces some duplication, and you can still add a # SAVE ACTIVITIES to make it stand out. :-) Actually, I didn't take it far enough - it should have been: 
      for (@activity) {
    print SAVE "[Button]\n";
    my $item;
    print SAVE "$prop=" . $_->$prop() for $_->properties;
}

      [download]
      where properties() simply looks like sub properties { qw(legend time elapsed) } and then the INI saving loop doesn't require any maintance at all any more.

      Makeshifts last the longest.

[reply]
[d/l]
[select]

In Section Meditations