tags:

views:

151

answers:

6

I was wondering if anyone had any suggestions on improving the following code (if possible) so that it didn't need the repeated (my @a = $time =~ ...), possibly using case/switch or given/when or some other idea that i'm missing?

my $time = '12:59pm';

if( my @a = $time =~ m/^(\d\d?)(am|pm)$/ )        { tell_time( $a[0], 0, $a[1] ) }
if( my @a = $time =~ m/^(\d\d?):(\d\d)(am|pm)$/ ) { tell_time( @a ) }
if( my @a = $time =~ m/^(\d\d?):(\d\d)$/ )        { tell_time( @a ) }

sub tell_time
{
    my $hour    = shift;
    my $minute  = shift || '00';
    my $ampm    = shift || ( $hour > 12 ) ? 'pm' : 'am';

    print "Hour: $hour, Minute: $minute, AMPM: $ampm\n";
}

I've tried playing around with Switch and the 5.10 given/when but can't seem to be able to do something like:

given( $time )
{
    when( /^(\d\d?)(am|pm)$/ )        { tell_time( $_[0], 0, $_[1] ) }
    when( /^(\d\d?):(\d\d)(am|pm)$/ ) { tell_time( @_ ) }
    when( /^(\d\d?):(\d\d)$/ )        { tell_time( @_ ) }
}

That doesn't fly because @_ appears to be storing $time.

also note I'm more interested in the syntax of the problem than the problem the code solves. I'm well aware that I could use Time::ParseDate to figure out the various parts of a string formatted like a time or date.

+9  A: 

Your regex uses () to extract matches, but you don't have to store these in an array. If you want, they're stored in $1, $2, $3, and so on. Lookie:

given( $time )
{
    when( /^(\d\d?)(am|pm)$/ )        { tell_time( $1, 0, $2 ) }
    when( /^(\d\d?):(\d\d)(am|pm)$/ ) { tell_time( $1, $2, $3 ) }
    when( /^(\d\d?):(\d\d)$/ )        { tell_time( $1, $2 ) }
}

Does exactly what I think you want to do.

If you want to add to the syntax, I would write tell_time() to simply take the time as a string, and have the function parse the result itself, rather than make the user of your code parse it himself. Alternatively, you could use this given() block as the start of a new function that does exactly that - parses a time string and passes it correctly to tell_time(). But that's just me. I don't know what you need your code to do, so by all means go for it.

Chris Lutz
Thanks for the head shake. I first tried exactly that with Switch.pm where it does not work and I guess I overlooked trying the same thing with given/when.In terms of further improving the function, the whole example was actually a re-write of a code sample from Pragmatic Programmer whose concept interested me, but was not particularly fond of the code implementation.Thanks for your help.
unobserved
+1  A: 

Well, without using switch/case, I'd just use a single regex to capture all the variations...

#!/usr/bin/perl

tell_time ("12:59am");    # matches time format 1
tell_time ("2:59pm");     # matches time format 1
tell_time ("12am");       # matches time format 2
tell_time ("12:59");      # matches time format 3
tell_time ("14:59");      # matches time format 3
tell_time ("12:59:59am"); # produces no output, does not match any known time formats.

sub tell_time
{
    my $timearg = shift;

    # note: (?: ... ) creates a non-capturing group, which is not reflected in 
    # the returned array.
    my ($hour , $minute, $ampm) = ( $timearg =~ m/^(\d\d?)(?::(\d\d?))?(am|pm)?$/ ) ;

    # only continue if we captured all required fields (i.e. hour)
    if($hour)
    {
        # set default values for optional fields (i.e. minute, ampm) if necessary
        $minute ||=  '00';
        $ampm ||=  ( $hour > 12 ) ? 'pm' : 'am';

        print "Hour: $hour, Minute: $minute, AMPM: $ampm\n";
    }

}

I can explain it further if necessary, but I think if you can read perl it should be clear what it's doing...

Stobor
I suppose that last one is supposed to fail? It doesn't fail where you think it should: "Use of uninitialized value $hour in numeric gt (>) at - line 23."
Chris Lutz
@Chris Lutz: Good point, that last one was supposed to produce no output, which it does for me... I should move that test earler... But interesting that it doesn't fail for me. /me goes off to check that.
Stobor
My impression is that the original poster is interested in switch-like syntax. Using a single regex defeats the purpose of the exercise.
Michael Carman
Actually, it doesn't fail, it just prints an error, and (for me) only if using -w (which everyone, including me, should use!). Will update to eliminate that warning.
Stobor
It produces no output, but it does give an error - if $hour is undefined (and you're using strict and warnings - but you always do that) then "$ampm ||= ( $hour > 12 ) ? 'pm' : 'am';" will technically work, but if the regex failed, $hour is uninitialized, and so it will issue a warning in code that uses warnings. You could fix it by adding a line like "$hour //= 0;" (if $hour is undefined, set it to 0) or changing "( $hour > 12 )" to "( defined($hour) and $hour > 12 )"
Chris Lutz
Okay, you got it without my help, and with your own solution. Cool deal, and excellent example of TMTOWTDI.
Chris Lutz
@Michael Carman: Fair point. I read the question as "improve the code so that it doesn't need the repeated bits". This is one way of achieving that, if the regexes are combinable in a sensible way as in this case.
Stobor
@Chris Lutz: Thanks, yeah, I recognized the warning straight away, but was confused as to why I didn't see it in my quick test. /me needs to "echo PERL5OPT=-w >> /etc/bashrc" someday soon. :-)
Stobor
A: 

Chris Lutz already covered the switch syntax using Perl 5.10. In order versions of Perl you can use loop aliasing to emulate one:

for ($time) {
  /^(\d\d?)(am|pm)$/        && do { tell_time( $1, 0, $2 );  last };
  /^(\d\d?):(\d\d)(am|pm)$/ && do { tell_time( $1, $2, $3 ); last };
  /^(\d\d?):(\d\d)$/        && do { tell_time( $1, $2 );     last };
}
Michael Carman
Isn't there a Switch module that emulates switch statements using a source filter? Is this more or less what it does behind the scenes or what?
Chris Lutz
There is. I've never used it or looked at it's innards but I would presume that this is what how it works.
Michael Carman
This is also along the lines that I was looking for. A little more verbose than the 5.10 given/when, but still agreeable. And as for Switch.pm, as I mentioned in my comment to Chris's answer, I did try using $1, $2, $3, etc in it's switch/case statement but everything was coming back undef.
unobserved
+1  A: 

Since you are using 5.10, you might as well use named captures in your regex:

#!/usr/bin/perl

use 5.010;
use strict;
use warnings;

my $hour24   = qr/(?<hour>[1-9]|1[0-9]|2[0-3])/;
my $hour12   = qr/(?<hour>[1-9]|1[0-2])/;
my $minute   = qr/(?<minute>[0-5][0-9])/;
my $meridiem = qr/(?<meridiem>am|AM|pm|PM)/;

for my $time (qw(5pm 10am 5:59pm 10:00pm 5:00 22:00 24:00)) {
    given($time) {
     when(/ ^ $hour12 $meridiem $ /x) { 
      my $hour = $+{hour};
      $hour += 12 if 'pm' eq lc $+{meridiem};
      tell_time($hour, "00") 
     }
     when(/ ^ $hour12 : $minute $meridiem $ /x) { 
      my $hour = $+{hour};
      $hour += 12 if 'pm' eq lc $+{meridiem};
      tell_time($hour, $+{minute}) 
     }
     when(/ ^ $hour24 : $minute $ /x) { 
      tell_time($+{hour}, $+{minute}) 
     }
     default {
      say "bad time: $time";
     }
    }
}

sub tell_time {
    my ($hour, $minute) = @_;
    say "it is $hour:$minute";
}
Chas. Owens
I actually prefer military time too, but I think the OP would prefer to keep the display to AM/PM. However, I'm curious - are named captures any less efficient than unnamed ones? It seems a bit like there might be a (small) bit of a performance hit compared to unnamed ones, or is it all the same in the bytecode?
Chris Lutz
@Chris Lutz The implementation of tell_time was not specified, so I took the liberty of giving a simple implementation for test purposes. I don't know if there is a performance hit, time to benchmark.
Chas. Owens
@Chris Lutz My benchmark showed the normal captures are 20% faster.
Chas. Owens
A: 

I am not sure if the given/when aspect is important here. I would just combine the possible patterns in a single regex. Combined with the special variable %+ and the defined-or operator, we can make the code more succinct.

#!/usr/bin/perl

use strict;
use warnings;

my @times = qw( 12:59pm 12 1pm 13:11 11 11pm);

my $hour_pat   = '(?<hour>[0-9]{1,2})';
my $minute_pat = '(?<minute>[0-9]{2})';
my $ampm_pat   = '(?<ampm>am|pm)';

my $re = qr{
    \A
    (?:$hour_pat : $minute_pat $ampm_pat)
    |
    (?:$hour_pat : $minute_pat)
    |
    (?:$hour_pat $ampm_pat)
    |
    (?:$hour_pat)
    \z
}x;

for my $time ( @times ) {
    if ( $time =~ $re ) {
        tell_time( %+ );
    }
}

sub tell_time {
    my %time = @_;
    printf( "Hour: %2.2d, Minute: %2.2d, AMPM: %s\n",
        $time{hour},
        $time{minute} // 0,
        $time{ampm} // ( $time{hour} >= 12 ? 'pm' : 'am' ),
    );
    return;
}
Sinan Ünür
A: 

I create switch with a block label, like this:

my $time = '12:59pm';
SWITCH: {
    $time =~ /^(\d\d?)(am|pm)$/ && do { 
     tell_time($1,0,$2);
     last SWITCH;
    };
    $time =~ /^(\d\d?):(\d\d)(am|pm)$/ && do {
     tell_time($1,$2,$3);
     last SWITCH;
    };
    $time =~ /^(\d\d?):(\d\d)$/ && do {
     tell_time($1,$2);
    };
}
Fran Corpier