[GingerC]

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:

ACTION=MbxGet,DEST_OR_SRC_SVR=WPSP,IN_MBX=/DWH/Upload,FILE_LIST=test_ftp_wpsp.lst, LOCAL_DIR=/datax/srv/dwh/outbound,FILE_TYPE=ASCII#$FILE_PATTERN=test_ftp_wpsp.txt

I would need to create a parameter ZIP='Y' in the current Perl script, and call a subroutine "Zip" that is already created in an old script.
Here is what I have so far:

#new params added to control record
GMIoutl::Zip($params{ZIP});

#If the control record states ZIP='Y', then zip the file. 
if ($params{ZIP} = "Y" {
				&Zip;
			}

#Subroutine from Perl Module: GMIoutl.pm
sub Zip
{

	my $self = FormatSelf(@_);
	my $func_exit = 0;	
	my $badcnt;
	my $numfiles;

	if (!defined $self->{SourceDir})
	{
		print "Could not determine source file directory <$self->{SourceDir}>\n";
		return 1;
	}
	if (!defined $self->{InputFileName})
	{
		print "Could not determine source file name <$self->{InputFileName}>\n";
		return 1;
	}
	if (!defined $self->{DestDir})
	{
		print "Could not determine destination file directory <$self->{DestDir}>\n";
		return 1;
	}
	if (!defined $self->{OutputFileName})
	{
		print "Could not determine destination/target file name <$self->{OutputFileName}>\n";
		return 1;
	}

	print "SourceDir				= $self->{SourceDir}   \n";
	print "DestDir					= $self->{DestDir}     \n";
	print "Wild card InputFileName/Single Filename	= $self->{InputFileName}     \n";
	print "OutputFileName				= $self->{OutputFileName}  \n";

	
	#********************************************************************
	#* Open the directory which contains the files to be zip/compressed *
	#********************************************************************

	opendir(CMDDIR, "$self->{SourceDir}") || $badcnt++;
        
	if ($badcnt > 0)
	{
		print "ERROR: Cannot open <$self->{SourceDir}> directory for processing. \n";
		return 1;
	}
	else
	{
		#****************************************************************************
		#* Read the files containing the search string in the directory into a list *
		#****************************************************************************
                
		@cf = readdir(CMDDIR);
		foreach (@cf)
		{
			if (/$self->{InputFileName}/)
			{	
				#create an array containing the desired filenames
				push(@FILES,$_);
			}					
		}
                
		$numfiles = @FILES;
		print "INFO: Number of files to be ZIP'ed/Compressed are $numfiles \n\n";
		closedir(CMDDIR);
                
		if ($numfiles == 0)
		{
			#**************************************
			#* There are no files to zip/compress *
			#**************************************
			
			print "ERROR: No files in <$self->{SourceDir}> directory to zip. \n";
			return 1;
		}
		else
		{
			if (-e "$self->{DestDir}/$self->{OutputFileName}.zip")
			{
				print("INFO: <$self->{OutputFileName}.zip> file is FOUND in <$self->{DestDir}> directory. \n");
				print("INFO: Deleting <$self->{OutputFileName}.zip> file from <$self->{DestDir}> directory. \n\n");
				
				$func_exit = system ("rm -f  $self->{DestDir}/$self->{OutputFileName}.zip");
				if ($func_exit)
				{
					print("ERROR: Unable to delete <$self->{OutputFileName}.zip> file from <$self->{DestDir}> directory. \n");
                                        return 1;
                                }
			}
			
			print("INFO: Creating a new <$self->{OutputFileName}.zip> file in <$self->{DestDir}> directory. \n");
			
			foreach (@FILES)
			{
				$func_exit = system ("zip -j  $self->{DestDir}/$self->{OutputFileName}.zip $self->{SourceDir}/$_");
				if ($func_exit)
				{
					print("ERROR: Unable to zip/compress <$_> file into <$self->{OutputFileName}.zip> file. \n");
					return 1;
				}
			}
		}
	}

	if (!$self->Silent())
	{
		print "End GMIoutl.Zip\n";
	}

	return $func_exit;

} #end of Zip sub

Am I even doing this right? Any suggestions?? Any help would be much appreciated!

[dsherohman]

GingerC, on 21 March 2013 - 03:36 PM, said:

Hi everyone! I am very new to coding so please be patient.

Hello and welcome aboard!

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:

opendir(my $cmd_dir, $self->{SourceDir});  # Removed useless use of quotes
unless (defined $cmd_dir) {
  print "ERROR: Cannot open <$self->{SourceDir}> directory for processing: $! \n";
  return 1;
} else {

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.
---

    @cf = readdir(CMDDIR);
    foreach (@cf) {
      if (/$self->{InputFileName}/) {
        #create an array containing the desired filenames
        push(@FILES, $_);
      }
    }

This can be simplified to:

@FILES = grep /$self->{InputFileName}/, readdir($cmd_dir);

---

        $func_exit =
          system("rm -f  $self->{DestDir}/$self->{OutputFileName}.zip");

You can do this directly within Perl:

unless (unlink "$self->{DestDir}/$self->{OutputFileName}.zip") {
  print "ERROR: Unable to delete <$self->{OutputFileName}.zip> file from <$self->{DestDir}> directory: $! \n"
  return 1;
}

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.
---

        $func_exit = system(
          "zip -j  $self->{DestDir}/$self->{OutputFileName}.zip $self->{SourceDir}/$_"

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:

use Archive::Zip ':CONSTANTS';
my $zip = Archive::Zip->new;
$zip->add_file("$self->{SourceDir}/$_");
unless($zip->writeToFileNamed("$self->{DestDir}/$self->{OutputFileName}.zip") == AZ_OK) {
  print "ERROR: Unable to zip/compress <$_> file into <$self->{OutputFileName}.zip> file. $!\n"
}

(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

if ($something_went_wrong) {
  print "Error message";
  return 1;
} else {
  # do stuff
}

multiple times.

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:

if ($something_went_wrong) {
  print "Error message";
  return 1;
}

# do stuff

instead.


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.