HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Sleep with count down display

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withsleepdowncountdisplay

Problem

I have a sleep function which displays the time remaining in sec, some feedback would be appreciated

use warnings;

$x=$ARGV[0]; 
$|++;

$n=0; 
# arguments could be 1, 1s , 1m or 1h
if($x=~/([0-9]+)([a-zA-Z]?)/) 
{ 
   $t = lc($2) ; 
   if($t eq "h"){$n = $1*3600;} 
   elsif( $t eq "m" ){$n = $1*60;}
   elsif( $t eq "s" || $t eq "") {$n = $1;} 
   else {die;}
} 
else {die;} 

while($n)
{ 
   print $n; 
   sleep 1; 
   $len=length($n); 
   print "\r", " "x$len, "\r"; $n--;
}


This code and updates are available at csleep

Solution

Naming

The names are very poor. Consider these renames:

  • n -> seconds



  • t -> unit



  • x -> input



Make the most out of regex

If you only allow "s", "m", or "h" as units, then instead of [a-zA-Z] you could use [smhSMH] in the regex.

Formatting

The formatting is really too compact. It's recommended to put spaces around operators.

use strict

It's recommended to use strict always.
It can help catching bugs.

Alternative implementation

Applying the above suggestions (and a bit more), consider this alternative implementation:

use warnings;
use strict;

my $seconds; 

# input could be 1, 1s , 1m or 1h
my $input = $ARGV[0]; 
if ($input =~ /^([0-9]+)([smh]?)$/i) { 
    $seconds = $1;
    my $unit = lc($2); 
    my $multiplier = 1;
    if ($unit eq "h") {
        $multiplier = 3600;
    } elsif ($unit eq "m") {
        $multiplier = 60;
    }
    $seconds *= $multiplier;
} else {
    die;
} 

$| = 1;  # force flush output on every write
while ($seconds) { 
    print $seconds; 
    sleep 1; 
    my $len = length($seconds); 
    print "\r", " " x $len, "\r"; $seconds--;
}

Code Snippets

use warnings;
use strict;

my $seconds; 

# input could be 1, 1s , 1m or 1h
my $input = $ARGV[0]; 
if ($input =~ /^([0-9]+)([smh]?)$/i) { 
    $seconds = $1;
    my $unit = lc($2); 
    my $multiplier = 1;
    if ($unit eq "h") {
        $multiplier = 3600;
    } elsif ($unit eq "m") {
        $multiplier = 60;
    }
    $seconds *= $multiplier;
} else {
    die;
} 

$| = 1;  # force flush output on every write
while ($seconds) { 
    print $seconds; 
    sleep 1; 
    my $len = length($seconds); 
    print "\r", " " x $len, "\r"; $seconds--;
}

Context

StackExchange Code Review Q#127456, answer score: 7

Revisions (0)

No revisions yet.