in reply to First Unix Admin Script - Request for Constructive Critisism

First, you really must "use strict" and "use warnings". There is a serious bug in your script that strict would have caught for you. The $scan in: my $scan = 1; is not the same as the $scan in: if ($scan) { The latter is actually $main::scan, since the former is out of scope, so your script will never obey "-s".

Another problem: You don't test for success when you open MPCFG, and you open it without closing it. You could run out of open file handles, for one thing. Packages IO:File or FileHandle should be preferred for file handles; if you put them in my variables they close themselves.

Some of the other things I noticed are a matter of style. I would combine the print messages before "die" with the die command itself. You print both ordinary trace messages and errors to STDOUT. Since this is a system utility you might want better control over logging and message levels; see Log::Log4perl.

But please "use strict"!

Replies are listed 'Best First'.
Re: Re: First Unix Admin Script - Request for Constructive Critisism
by grantm (Parson) on Feb 08, 2003 at 20:38 UTC

    One minor nit pick:

    You don't test for success when you open MPCFG, and you open it without closing it. You could run out of open file handles

    Filehandles are global (another good reason for preferring IO::File as you suggest) so if open is called with a FILEHANDLE argument that refers to an already open file, the FILEHANDLE is closed first. The original script might not be good style, but it doesn't leak filehandles.

[reply]
      Thanks for the head up on IO:File. You mention that the script isn't good style. I would appreciate any advise, in this area, as I am just leveraging perl into what I have seen in ksh.
[reply]

        The comment about style merely related to calling open() but not calling close(). In your script, it's really not a problem - each subsequent call to open() will close the previously opened file since the same filehandle is used each time. The last filehandle will be closed when the script exits.

        IO::File allows you to store your filehandle in a normal scalar - which is especially useful if you want to pass it to a subroutine. You would typically use a lexically scoped scalar (one declared with 'my') so that the filehandle was not global.

        Regardless of which form of open you use, as tall_man said, you should always check the return value and at the very least call die "$!" if it fails.

[reply]
[d/l]

In Section Seekers of Perl Wisdom