Whatever you do, please don't consider using a variable as a variable name.

Instead you should look at complex data structures like arrays of arrays. See perlreftut, perllol and perldsc for details.

Code review notes follow:

opendir (*LOWEST, "$input_directory_lowest")
[download]

Please take another look at the documentation for opendir. You are still getting it wrong. You pass it the name of a directory handle (LOWEST) not a typeglob (*LOWEST). And there is no need to quote the variable $input_directory_lowest.

if ($do_I_pick_up_request == 'Y')
[download]

And how many times have people tried to explain the difference between string and numeric comparison operators to you?

I think you're definitely on the track, to want to generalize the code to any number of directories. The first thing to do is try to eliminate variables — or rather, variable names — which perpetuate the "one chunk of code per directory" mentality.

Below is how you might reduce it to a loop.

sub Prioritise_requests
{
  my( $input_directory, $node ) = @_;

  for my $dir_num ( '1', '2', '3' )
  {
    my $input_dir = $input_directory.$dir_num;

    opendir DIR, $input_dir or die "Error opening directory $input_dir
+ - $!";
    my @files =
      sort { -M $a <=> -M $b }
      map "$input_dir/$_",
      grep /^Flat_file.*\.txt$/,
      readdir DIR;
    closedir DIR;

    foreach ( @files )
    {
      if ( Is_the_file_mine($node, $_) and Do_I_run_the_request_now($_
+) )
      {
        move $_, $input_directory or warn "Error moving $_ to $input_d
+irectory - $!";
        return;
      }
    }
  }
}
[download]

I've also made one critical bug fix in the code above: You have to add the directory name to the file name before you get any of the file's information, e.g. -M.

Also, I made a change as to the assumed return values of the two hypothetical subroutines Is_the_file_mine and Do_I_run_the_request_now. You were assuming they'd return characters. But really, they return boolean (true/false) conditions. May as well just code for that directly.

sub Prioritise_requests {

  my ($input_directory, $node) = @_;

  my %files;
  $files{1} = $input_directory."1";
  $files{2} = $input_directory."2";
  $files{3} = $input_directory,"3";

  # this is looping on the directories in a prioritized manner
  foreach (sort keys %files) {
        open(*DATA,$_) or die;
        # this is looping through each file you want in each directory
        foreach (sort grep(/^Flat_file.*\.txt$/,readdir(DATA))) {
                # do your stuff for each file
        }
        close(DATA);
  }
}
[download]

Hi, I understand that what you wrote is marked as "pseudocode", but I noticed a couple of strange things:

$files{3} = $input_directory,"3";
[download]

Maybe you meant to put a dot "." instead of a comma ","?

Another thing that I don't understand from this fragment is if %files holds filenames or directory names. The use of open() and close() seems to point to "filenames", but in this case the readdir(DATA) should be simply <DATA>.

Regarding the open(), at the price of backward compatibility with 5.6, I use lexical filehandles instead of barewords:

   open my $data, ...
[download]

Moreover, I usually stick to the three argument version of open(), not because this is requested by this particular application (I actually don't know), but more for sake of a mental habit (yes, I'm mental :) :

   open my $data, '<', $files{$_} ...
[download]

I find it better not only because it avoids nasty problems when the filename comes from the "untrusted external" (which could lead to embarassing security holes), but also because it visually marks that this open is for read instead of write.

Last, even if the plain die does the work (especially in pseudocode!), it could be helpful to provide a more sensible feedback message, either directly:

   open my $data, '<', $files{$_} or die "open('$files{$_}'): $!";
[download]

or with

use Fatal qw( open );
[download]

in the script's preamble.

Forgive me for jumping on your shoulders this way, but the OP seems to have some bias towards blind cut-and-paste, you know, so it could be useful for her|him to think about these issues ;)

Flavio
perl -ple'$_=reverse' <<<ti.xittelop@oivalf

Don't fool yourself.

Regarding the open(), at the price of backward compatibility with 5.6, I use lexical filehandles instead of barewords:

Lexical handles (and 3-arg open) work fine in 5.6.

>c:\progs\perl560\bin\perl script.pl
open(my $data, '<', $0) or die;
print while <$data>;
[download]

Lexical handles were introduced in 5.6, so you are sacrificing backwards compatibility wih 5.5 (5.005).

What was introduced in 5.8 is file handles to variables (open(my $fh, '<', \$buf)).

Heh, no worries. I wasn't exactly trying to meet stellar code standards. It was the best I could spit out in about 30-40 seconds ;)

That open(DATA) line should be:

open(*DATA,$files{$_}) or die;
[download]

You get the point though.

Or, even better:

open(DATA, $files{$_}) or die $!;
[download]

Removed unnecessary typeglob symbol and added error message.

--


<http://dave.org.uk>

"The first rule of Perl club is you do not talk about Perl club." -- Chip Salzenberg

Question: In the code above, you only do anything (processing files in any folder) if there is something in the high-priority folder. Specifically, you have:

  if (@sorted_highest_priority) {
    foreach (@sorted_highest_priority){
      ...
    }
    
    if (@sorted_middle_priority) {
      foreach (@sorted_middle_priority){  
        ...
      }
    }
    
    if (@sorted_lowest_priority) {
      foreach (@sorted_lowest_priority){  
        ...
      }
    }
  }
[download]

That's a mistake, right?

my @folders = glob 'input_directory*'; # get/sort all qualifying folde
+rs
my @requests;
for (my ($found, %filedate); $found = pop @folders or exit();) {
    @requests = sort {
      { ( $filedate{ $a } ||= -M $a )
      <=> ( $filedate{ $b } ||= -M $b )
      } glob "$found/" . 'Flat_file.*.txt'
        and last; # reiterate iff none found
}
[download]

A hash of prioritised lists seems like what you need:

use warnings;
use strict;

my $inputdir;
my %files = Prioritise_requests ($inputdir);

for my $fileList (sort keys %files) {
    print "$files{$file}->[0]\n"; # Lowest priority file
    print "$files{$file}->[-1]\n"; # Top priority file
}

sub Prioritise_requests {
    my ($input_directory, $node) = @_;
    my %files;
    my $index = 1;
    
    while (1) {
        last if ! opendir SCAN, "$input_directory$index";
        
        for my $filePath (grep (/^Flat_file.*\.txt$/, readdir (SCAN)))
+ {
            my ($file) = $filePath =~ m![^\\/]*$!;
            push @{$files{$file}}, $_;
            ++$index;
        }

        closedir SCAN;
    }

    return %files;
}
[download]

Note that this is untested and doesn't include all the "non-issue" work of your sample code.

Updated to avoid "perpetual" loop