Can I clean this up??

[Anonymous Monk]

I would like to know if I could clean this up and get rid of my repititious lines such as the system(mail...) commands and anyother way to compact this?? I also tried playing with my open FIL and close FIL but could only get them to work as I have it below:

My script checks how many systems are down so the $ct variable represents how many systems are down.
Here is the part I was hoping I could condense:

if($ct == 1)
{
        print FIL "\n$ct is down at this time.\n";
        open(FIL,">>$myfil") || die "No open for ct tot: $! \n";
        system("mailx -s 'Mail header' myemail < $myfil");
        close(FIL);
}
elsif($ct >= 2)
{
        print FIL "\n$ct are not available at this time.\n";
        open(FIL,">>$myfil") || die "No open for ct tot: $! \n";
        system("mailx -s 'Mail header' myemail < $myfil");
        close(FIL);
}
close(FIL);

[frankus]

First off consider what the if statement is doing:

open(FIL,">>$myfil") || die "No open for ct tot: $! \n";
if($ct == 1){
        print FIL "\n$ct is down at this time.\n";
}
elsif($ct >= 2){
        print FIL "\n$ct are not available at this time.\n";
}
system("mailx -s 'Mail header' myemail < $myfil");
close(FIL);


Secondly consider using a pipe instead of writing to a file:

open(MAIL,"|mailx blah) || die "No mail stuff: $! \n";
if($ct == 1){
        print MAIL "\n$ct is down at this time.\n";
}
elsif($ct >= 2){
        print MAIL "\n$ct are not available at this time.\n";
}
close(MAIL);

Try some common Perl tricks:

# Removed brackets, replaced || with or due to higher presedence and r
+eadability 
# used single quotes on uninterpolated strings.
open MAIL,'|mailx blah' or die "No mail stuff: $!\n";

# Stuck the if statements at the end so it's more natural.
print MAIL "\n$ct is down at this time.\n" if $ct == 1;
print MAIL "\n$ct are not available at this time.\n" if $ct >= 2;

close(MAIL);


What? more elegant?.....um, it's arguable.

open(MAIL,"|mailx blah) || die "No mail stuff: $! \n";
SWITCH: {
   $ct >= 2   && { print MAIL "\n$ct are not available at this time.\n
+"; last SWITCH };
   $ct == 1   && { print MAIL "\n$ct is down at this time.\n"; last SW
+ITCH };
}
close(MAIL);

--

Brother Frankus.

¤

[PhiRatE]

Nice work. Lets add a bit of holistic consideration. What exactly is he trying to achieve? well, very simply it's a plural translation within english. In those instances where more than one system is down, he wishes to maintain a correct english sentence.

We thus minimise our code duplication like this (perl):

open(MAIL,"|mailx blah") || die "No mail stuff: $! \n";
if ($ct>0) {
    print MAIL "\n$ct ".($ct>1 ? "are not available" : "is down")." at
+ this time.\n";
}
close(MAIL);

But of course the greatest optimisation is always achieved by considering the whole problem, not just the perl:

open(MAIL,"|mailx blah") || die "No mail stuff: $! \n";
print MAIL "\nSystems unavailable at this time: $ct\n" if ($ct>0)
close(MAIL);

Silly I know, but to the point: Good optimisation looks at the whole problem.

[Anonymous Monk]

Wow, thanks for all the suggestions! Now I can make it look better.

[tadman]

It's a simple case of refactoring. I really dislike duplication, since duplication is just asking for bugs. As a note, it seems you're writing to FIL before you open it.

if ($ct > 0)
{
     open(FIL, ">>$myfil") || die "Write to $myfil failed\n";

     print FIL "\n$ct ";
     print FIL ($ct > 1)? "are not available" : "is down";
     print FIL " at this time.\n";
     close (FIL);

     system("mailx -s 'Mail Header' myemail < $myfil");
}

Note that you should probably be using something like Mail::Mailer or Mail::SendMail to do the dirty work of sending mail.

In your original code, if your e-mail address changed, you'd have to do twice the work to update it, and further, there is a chance you might miss one of them.

When handling simple pluralization, like what you have here, the ?: operator comes in handy. If you're not familiar with it, here's how it works:

print "\$foo is ";
print $foo? "true" : "false";
print "\n";

Or you can compact this to something like so:

print "\$foo is ",($foo? "true" : "false"),"\n";

More idiomatically:

print "There ",(($num == 1)? "is" : "are")," $num camel",
       (($num == 1)? "" : "s")," for sale.\n";

Or perhaps something more "Old School":

printf("There %s $num camel%s for sale.\n",
       ($num == 1)? ("is","") : ("are","s"));

[Abigail-II]

Note that you should probably be using something like Mail::Mailer or Mail::SendMail to do the dirty work of sending mail.

I can think of reasons why in some cases you might prefer Mail::Mailer over using mailx, but "the dirty work" isn't one of them. When I use Mail::Mailer, I need three lines of code on overhead. One line for the OO overhead, a second line to actually open a mail, and a third line to close the mail.

When using mailx, I only need 2 lines: one for the open, a second for the close. There's no "dirty work" here - the dirty work is done by mailx!

I've never had problems using mailx, something I cannot say from Mail::Mailer (or Mail::Send).

Abigail

[tadman]

% which mailx
which: no mailx in (/bin:/usr/bin:/usr/bin/X11: ...

One case: RedHat does not install mailx by default, so you'd have to be careful about how you deploy this script. As for the modules, they may be more "expensive" in terms of lines of code and overhead, but I think you get what you pay for. And don't forget, system() calls aren't exactly free either.

To some extend, I agree with your point. system() calls in command-line scripts may be acceptable, especially for quick hacks. However, if you're talking in general terms, the modules are far more adaptable, especially in something like a mod_perl environment.

If you need to send mail, you better be using SMTP. Sure, you can use 'mail', but how can you tell if the system actually checks the mail queue. Is the mail daemon even running? Can you even assume it's going to be Sendmail? If you have your own little kingdom where certain things can be taken for granted, you can code accordingly. In general terms, though, such assumptions are risky.

[djantzen]

The only difference between the two branches is the message sent, so the thing to do is to move the common code into a subroutine specifially for sending mail. In each branch of the conditional you would call the subroutine, perhaps passing the desired message as a parameter.

Secondly, try using the module Mail::Send, which provides a simple and standard interface to various mail programs, rather than manually calling a mailer via system. You may find this discussion helpful as well: How do I rewrite mail headers using Mail::Send?.

[ackohno]

open(FIL,">>$myfil") || die "No open for ct tot: $! \n";
if($ct == 1)
{
        print FIL "\n$ct is down at this time.\n";
}
elsif($ct >= 2)
{
        print FIL "\n$ct are not available at this time.\n";
}
system("mailx -s 'Mail header' myemail < $myfil");
close(FIL);

That should function the same without repeating the opening, mailing, closeing.


Notice: At the time I was writeing this response frankus was writeing it too and beat me to submiting, heh. sorry for repeating.
--
($good, $bad, $ugly) = split(/,/, "vi,emacs,teco")

[Sifmole]

print FIL "\n$ct is down at this time.\n";
open(FIL,">>$myfil") || die "No open for ct tot: $! \n";

Uh, this is bass-ackwards -- you print to the file before you open it. That isn't going to work at all.

[dimmesdale]

You may want to check out Lingua::EN::Inflect -- it takes a singular form of a noun/verb/some adjectives and spits out the plural form.

From the docs, it can do it conditionally as such:

# CONDITIONALLY FORM THE PLURAL


      print "I saw $cat_count ", PL("cat",$cat_count), "\n";

Your conditional statement just wants to make the is/are look right for the end user -- now you've gotten helpful repiles for something that works specifically for the is/are part, but in the future if you need more flexibiilty you can consider the module.