4
\$\begingroup\$

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

\$\endgroup\$
1
4
\$\begingroup\$

Here are some points I would like to see improved

  • Always use strict, and use my to declare your variables as late as possible

  • use warnings is preferable to -w on the shebang-line or command-line, but you should use warnings 'all' to ensure that you get all the help that perl has to offer

  • The coding standards in perldoc perlstyle will make your code instantly accessible to most Perl professionals. You should follow them unless you have a very good reason to write your code differently

  • If you have multiple parameters then it is more concise to use a list copy to assign the values to local variables. There is no reason to do it differently for a single parameter, so my ($p1) = @ARGV is better than my $p1 = $ARGV[0]

  • The built-in variables are poorly-named, and you should avoid using them explicitly if you can. $|++ is much better as STDOUT->autoflush

  • Single-character identifiers are unacceptable, except for a few exceptions. $i, $j and $k are traditionally integer values and array indices; similarly $x, $y and $z are floating-point coordinates. $t may be used for a time value, and $l, $w and $h for length, width and height. $n is a number - usually a count - and $s may be a string. Pretty much anything else can be written more explicitly using a longer name. Note that Perl reserves $a and $b as internal variables for the built-in sort operator. They should never be used outside a sort block

  • If you are testing something that invalidates all of the rest of the program then you should check it straight away and die with an appropriate message string, rather than enclosing all of the rest of your code in a conditional block. So it would be better to rewrite if($x=~/([0-9]+)([a-zA-Z]?)/) { ... } else {die} as $x=~/([0-9]+)([a-zA-Z]?)/ or die "bad input". After that, the rest of your program can assume that the input is well-formed

  • Make sure to anchor your regular expressions. $x=~/([0-9]+)([a-zA-Z]?)/ will be true if $x contains a string that matches the pattern, like >>>0<<<<. Try it!

  • If you're using a regular expression, then get it to do all the work it can. Instead of [a-zA-Z]? when you are expecting only h, m, s, H, M or S, you can write [hms]? and use the /i (case-independent) modifier on the match. Thereafter, you don't have to consider the case where the pattern has matched any other letter

  • Make your constants clear. "Magic numbers" never help, and although you must rely on people knowing that there are sixty seconds in a minute, you don't need to use 3600 for the hour multiplier. 60 * 60 commmunicates a lot more

  • Consider using the /x modifier on regex patterns that are at all complex. It allows you to add whitespace to patterns so that you can lay them out better and make them clearer to read. /([0-9]+)([a-zA-Z]?)/ is clearer as / ( [0-9]+ ) ( [a-zA-Z]? ) /x

  • People familiar with Perl will expect to see parentheses used much more rarely than in most languages. They are used more like commas in natural languages, and an expression like lc($2) is more common as lc $2

  • I personally prefer or and and to || and &&. I think they read better, and they often remove the need for additional parentheses. But beware that they have much lower precedence than their symbolic equivalents

  • Don't use the trailing ++ and -- operators when you don't need them. They mean "increment (decrement) the variable and return the value before it was changed". Most often --$n is just fine. The post-fixed operators have become popular only because of the name of the language C++, and it isn't a good guideline

I have altered your program, following the guidelines above. I hope you agree that it's preferable this way

use strict;
use warnings 'all';

STDOUT->autoflush;

my ($input) = @ARGV;

$input =~ / ^ ( [0-9]+ ) ( [hms]? ) $ /ix or die qq{Invalid input "$input"};

my $unit = lc $2;

my $seconds;

if ( $unit eq 'h' ) {
    $seconds = $1 * 60 * 60;
}
elsif ( $unit eq 'm' ) {
    $seconds = $1 * 60;
}
elsif ( $unit eq 's' or $unit eq "" ) {
    $seconds = $1;
}

while ( $seconds-- ) {

    print $seconds;
    sleep 1;

    my $len = length $seconds;
    print "\r", " " x $len, "\r";
}

I would personally choose to set up a hash that converted suffix strings to a multiplying factor instead of using a chain of if statements. Like this

use strict;
use warnings 'all';

STDOUT->autoflush;

my ($input) = @ARGV;

$input =~ / ^ ( [0-9]+ ) ( [hms]? ) $ /ix or die qq{Invalid input "$input"};

my %factors = (
    h  => 60 * 60,
    m  => 60,
    s  => 1,
    '' => 1,
);

my $seconds = $1 * $factors{ lc $2 };

while ( $seconds-- ) {

    print $seconds;
    sleep 1;

    my $len = length $seconds;
    print "\r", " " x $len, "\r";
}

I hope that helps

\$\endgroup\$
6
\$\begingroup\$

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--;
}
\$\endgroup\$
4
  • \$\begingroup\$ Hey your program doesn't work, I corrected some of them but they still have some problems, like argument 2a is still valid while it shouldn't be. ([smh]?) does match smh and that it is optional but if there is other letters than them they dont match but the the condition would be still true. this would need something like ([has smh] and [doesnt have other letters]) ideone.com/wGO6x6 \$\endgroup\$ – tejas May 5 '16 at 8:11
  • \$\begingroup\$ @tejas my bad, corrected \$\endgroup\$ – janos May 5 '16 at 8:16
  • \$\begingroup\$ wouldn't "2A" be a valid input still? \$\endgroup\$ – tejas May 5 '16 at 12:18
  • \$\begingroup\$ No, 2A is not valid, but 2A1s is. There should be a ^ anchor in the beginning of the pattern. \$\endgroup\$ – simbabque May 10 '16 at 9:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.