1 Replies - 8795 Views - Last Post: 22 March 2013 - 05:12 AM

Call Zip subroutine in an 'if' statement; task

Posted 21 March 2013 - 07:36 AM

Hi everyone! I am very new to coding so please be patient. I am currently an intern in an IT department and was given a task to try to find a way to compress a file before it transfers from one server to the other (internal to external-SFT). In order to transfer files, we use control records that look similar to this:

Re: Call Zip subroutine in an 'if' statement; task

A general tip first which will make the process of learning Perl much more pleasant, or at least much easier: Always start programs (after the #!/usr/bin/perl line) with:

use strict;
use warnings;

They will catch and notify you of a broad range of problems with your code - including an error or two that I see in the code you posted.

Now, onward to your code!

if ($params{ZIP} = "Y" {
&Zip;
}

There are two problems here (aside from the missing parenthesis):

1) if ($params{ZIP} = "Y") sets $params{ZIP} to "Y", then always returns true (because "Y" is a true value). If you had turned "warnings" on, this would generate the warning "Found = in conditional, should be ==" to inform you of this problem, although that's not quite accurate - since you're doing a string comparison, you actually want eq, not ==. (Although, if you used ==, you'd get a different warning for using == to compare non-numeric values, so "warnings" would still get you where you needed to go.)

2) Calling user-defined subs by prefixing them with & is a holdover from Perl 4 and has non-obvious side-effects. The preferred practice in modern versions of Perl would be to call it as Zip() if you were using it as a regular function. However, the use of $self in the Zip code suggests that it's actually an object method, in which case you should be calling it as $some_object->Zip.
---

my $self = FormatSelf(@_);

That is... very strange. I can't say that it's wrong, since I have no idea what FormatSelf does, but the normal way for an object to find itself in Perl is

my $self = shift;

---

opendir(CMDDIR, "$self->{SourceDir}") || $badcnt++;

Using bareword dirhandles is considered to be deprecated. The preferred option is to use a lexical dirhandle, like so:

Note that this also eliminates the need for $badcnt, so I redid the error-checking block for it, too. While I was at it, I added $! to the error message if the directory open fails. $! contains the operating system's error message, so this change will cause it to tell you why the directory couldn't be opened.
---

Keeping it within Perl is a little more efficient (it avoids opening a new shell process to run the external command) and also avoids any potential security issues (e.g., if a malicious person were to install a rogue "rm" binary on your path ahead of the real one), so I recommend doing so when possible.
---

Keeping this within Perl is also good for the same reasons as I mentioned above, but Perl doesn't have a built-in "zip" function. However, we do have CPAN and several good modules implementing zip file compression, such as Archive::Zip. If you install that, you can then do:

(Disclaimer: I've never actually used Archive::Zip myself. This code should be correct, or close to it, based on the Archive::Zip documentation, but I could be wrong.)
---
Finally, as a general point of style, you use the pattern

While this works, there's no need for the else block. Given that any code after the return won't be executed anyhow, it just pushes the indentation a bit further over for no good reason. Most programmers find it easier to read code with fewer levels of indentation, so it's generally preferred to use:

So there are the things I see that could be improved in your code. As for whether you're actually doing it right or not, you'll need to run it and see whether it does what you intended or not - but it looks to me like it probably should do what you want it to.