in reply to Radoteur

Hello QuillMeantTen,

If you see any improvements or corrections to my code, please do tell me so

Well, since you asked ... :-)

  1. You should always test calls to open and close for failure. Either append an explicit or die ... to each call, or just put use autodie at the head of the script.
  2. This statement: 
                           printf STDERR "we have cycled: doing line $. le
+tter 0
                       again\n";

    [download]
    prints a newline character followed by 24 spaces between 0 and again\n. This is almost certainly not what you intended.
  3. Reserve die (which is Perl’s way of throwing an exception) for error conditions; don’t use it for normal program termination.
  4. Don’t use goto, that way leads to spaghetti code! Recast the program logic to use a standard (structured) looping construct.
  5. Don’t use printf when print will do exactly the same job.

Here’s my re-write of the code. In addition to the points noted above, I’ve streamlined the script’s control flow. Comments have been removed:

#! perl
use strict;
use warnings;
use autodie;

die "To use me, give me the word list file as argument\n"
    unless defined $ARGV[0];

my %letter_lines;
my $evt;
my $niter = 0;

for (my $cycled = 0;
       !$cycled;
        warn "going back to the beginning\n" unless $cycled)
{
    open my $fh, '<', $ARGV[0];

    FILE_ITER:
    while (my $word = <$fh>)
    {
        my @letters = split '', $word;
        ++$niter;

        if (defined $evt)
        {
            WORD_ITER:
            for my $i (0 .. $#letters)
            {
                if ($evt eq $letters[$i])
                {
                    my $j = $i + 1;
                    $evt  = $i < $#letters ? $letters[$j] : undef;

                    if (defined $evt)
                    {
                        if (defined $letter_lines{$.}{$j})
                        {
                            warn "we have cycled: line $. letter $j\n"
+;
                            $cycled = 1;
                            last FILE_ITER;
                        }

                        warn "adding to hash line $. letter $j\n";
                        $letter_lines{$.}{$j} = 1;
                        print $evt;
                    }

                    last WORD_ITER;
                }
            }
        }
        elsif (defined $letter_lines{$.}{0})
        {
            warn "we have cycled: doing line $. letter 0 again\n";
            $cycled = 1;
            last FILE_ITER;
        }
        else
        {
            $letter_lines{$.}{0} = 1;
            warn "adding to hash line $. letter 0\n";
            $evt = shift @letters;
            print $evt;
        }
    }

    close $fh;
}

warn "cycled in $niter iterations\n";

[download]

Tested with “linuxwords.txt” downloaded from https://users.cs.duke.edu/~ola/ap/linuxwords. With allowances made for changes to certain print statements (see point (2), above), the outputs to STDOUT and STDERR are identical for the original and revised scripts.

Hope that helps,

Athanasius <°(((>< contra mundum Iustus alius egestas vitae, eros Piratica,

Replies are listed 'Best First'.
Re^2: Radoteur
by QuillMeantTen (Friar) on Oct 19, 2015 at 06:51 UTC

    Great many thanks for your input, I have started rewriting my code before even reading all your post, so here is my take on your advice:

    Read more... (4 kB)
[reply]
[d/l]

      Your loop labels are not required here (and rarely ever are). You can remove them in both the loops, and the last and next calls. For example, replace:

      WORD_ITER:foreach(...)

      [download]

      with:

      foreach(...)

      [download]

      and:

      next WORD_ITER;
last WORD_ITER;

      [download]

      with just:

      next;
last;

      [download]

      You can do this in both your while and for statements/blocks.

[reply]
[d/l]
[select]

        I know they are not needed, yet I read in Perl Best Practices that whenever last or next are used adding labels helps will the readability and future maintanability of the code

        It seemed to make sense so I adopted the practice. update: typo

[reply]

In Section Cool Uses for Perl