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

I am doing following to execute my command in the array and its works, as I am new kid doing programming in perl any one have any suggestion regarding my code? Please.
for (my $element = 0; $element<$total_element; $element++) { my ($run_cmd, $chdir_cmd) = split(';',$run[$element],2); ch_dir($chdir_cmd,$patch,'0'); if ($run_cmd =~ /check.pl/) { system($run_cmd) if ($? != 2) { print "Command Failed: While Executing $run_cmd$!\n"; $err = 1; } } else { system($run_cmd); if ($? != 0) { print "Command Failed: While Executing $run_cmd$!\n"; $err = 1; } } }
Any suggestion? Please Cheers

Replies are listed 'Best First'.
Re: looking for suggestion!!!
by Corion (Patriarch) on Jul 24, 2009 at 08:15 UTC
    for (my $element = 0; $element<$total_element; $element++)

    Don't. Use the following instead:

    for my $command (@run) { ...

    This is much saner, as you don't need to know about $total_element and cannot make errors by using $element in a wrong way.

    Also, please check the return value from system instead of only querying $?. Use the following:

    if (system( $run_cmd ) != 0) { print "Running command '$run_cmd' failed: $!/$?\n"; $err = 1; };
Re: looking for suggestion!!!
by GrandFather (Saint) on Jul 24, 2009 at 08:23 UTC

    Get a better keyboard that doesn't repeat punctuation characters.

    Don't use C for loops. Avoid rewriting the same code. Use English prose white space conventions where applicable. A little restructuring gives:

    use strict; use warnings; my @run; for my $element (@run) { my ($run_cmd, $chdir_cmd) = split ';', $element, 2; ch_dir ($chdir_cmd, $patch, '0'); system ($run_cmd); if ($run_cmd =~ /check.pl/) { next if $? == 2; } elsif ($? == 0) { next; } print "Command Failed: While Executing $run_cmd$!\n"; $err = 1; }

    which is of course incomplete and untested.


    True laziness is hard work
      Argument "cd /and/people/dir" isn't numeric in array element at release_patch.pl line 130. i am getting following warning how to get rid of it???

        use eq instead.


        True laziness is hard work
Re: looking for suggestion!!!
by ikegami (Patriarch) on Jul 24, 2009 at 08:27 UTC

    It seems your commands are broken, and you're spending effort trying to fix them at run-time. foo ; cd bar should be cd bar ; foo. You should fix your data before running your program, or at least separately from trying to execute them.

    Why is a check.pl dying from SIGINT with no coredump considered successful? Why is a check.pl dying from SIGINT with coredump considered unsuccessful? While is running a program successfully considered an error? You're probably killing the program when you should be closing its input stream with Ctrl-D (default for unix) or Ctrl-Z (Windows).

    You're printing $! when its meanlingless. You're not printing $? when its meanlingful.

    You are using a C-style loop when a Perl-style counting loop would be simpler. In fact, you are using a C-style loop when a foreach loop would be ideal.

    @run is both misnamed (it doesn't contain a run) and incorrectly pluralised (it s singular yet it contains multiple of what it contains).

    Basically, that whole snippet should be:

    for my $cmd (@cmds) { system($cmd) die("<$cmd>: Can't spawn: $!\n") if $? == -1; die("<$cmd> died from signal ", ($? & 0x7F), "\n") if $? & 0x7F; die("<$cmd> exited with code ", ($? >> 8), "\n") if $? >> 8; }

    Everything extra is you trying to fix bugs made elsewhere, bugs better fixed elsewhere since fixing those bugs have nothing to do with executing commands.

Re: looking for suggestion!!!
by marto (Cardinal) on Jul 24, 2009 at 08:41 UTC
Re: looking for suggestion!!!
by cdarke (Prior) on Jul 24, 2009 at 09:42 UTC
    In addition:
        if ($run_cmd =~ /check.pl/)is an unnecessary use of the regular expression engine (which is a really complex beasty).
    if ($run_cmd eq 'check.pl')
    is much easier.

    By the way, when submitting code please at least show how your variables are derived. Many have said that the C-style for loop is unnecessary, but actually we do not know what $total_element is. We can only guess it is the highest index number in @run

      Given the usage in the OP's code $total_element is the number of element rather than the largest index - this is why C style for loops are so prone to off by 1 errors. ;-)


      True laziness is hard work