6
\$\begingroup\$

This is the first thing I've written in Perl besides one-liners. It just generates random text based on a text file. I was inspired by this. Any tips on making this better are welcome. I'd especially like to know if there are more idiomatic ways I could do certain things.

#!/usr/bin/env perl
use strict;
use warnings;
use File::Open qw(fopen);
use Data::Munge;

my $k = shift;                  # k-gram length
my $n = shift;                  # number of iterations
my $file = shift;
my $long_string = slurp fopen $file;

# loop through each k-gram; if no entry exists in %markov_chain,
# initialize it to an empty array. In either case, push the next
# k-gram into the array.

my %markov_chain;               # hash of arrays

for my $i (0 .. (length $long_string) - 1) {
  my $curr = substr $long_string, $i, $k;
  my $next = substr $long_string, $i + 1, $k;
  $markov_chain{$curr} //= [];
  push $markov_chain{$curr}, $next;
}

# Take the first k-gram from the original document. Draw a random
# k-gram from this k-gram's entry in %markov_chain. This will be the
# k-gram for the next iteration. Print the last character of the
# k-gram.

sub rand_elem { $_[rand @_] }

my $seed = substr $long_string, 0, $k;
print $seed;
for my $i (0 .. $n) {
  $seed = rand_elem @{$markov_chain{$seed}};
  print substr $seed, -1;
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

The only thing that comes to mind is the way you open the file.

You could do this instead:

open(my $file, "<", "$ARGV[0]") or die;
$data = do { local $/; <$file> };
close $file;

Which is a well known Perl idiom to read an entire file in one go.

For more info see these links

https://perlmaven.com/open-files-in-the-old-way

https://perlmaven.com/slurp

http://www.perlmonks.org/?node_id=20235

\$\endgroup\$
1
\$\begingroup\$

Overview

It is great that you:

  • Used strict and warnings
  • Leveraged other people's code by using the CPAN modules
  • Added comments to describe aspects of the code

Here are some adjustments for you to consider, mainly for coding style.

Documentation

You need to add usage documentation to the code to explain its purpose, and its required inputs. It is standard practice to use plain old documentation (POD).

Compile errors

I realize this answer is posted 9 years after the question was asked. Many versions of Perl have come and gone in that time. If I run it with old versions 5.14 or 5.18, the code compiles without errors, but on newer versions like 5.24, there are compile errors.

One error is fixed by adding parentheses around the rand_elem call:

$seed = rand_elem(@{ $markov_chain{$seed} });

Another error is fixed by dereferencing the array reference in the following line:

push $markov_chain{$curr}, $next;

Use:

push @{ $markov_chain{$curr} }, $next;

Namespace

It is best to import only what is needed to avoid namespace pollution. For example, change:

use Data::Munge;

to:

use Data::Munge qw(slurp);

Parentheses

It is customary to use parentheses with module function calls. For example,

my $long_string = slurp fopen $file;

would look like:

my $long_string = slurp(fopen($file));

It is preferable to omit the parens for built-in functions as you have done:

shift
length
substr
etc.

Idiomatic

It is unusual to use 2 CPAN modules just to read in a file. The method you used in your original version of the question, also shown in the previous answer, is more idiomatic.

Indentation

It is preferable to use 4-space indentations than 2-space for readability.

Output

It is customary to print a newline after the last print statement so that the output is distinguished from the shell prompt after the code runs:

print "\n";

Style

I recommend moving the sub to the end of the code. Having it in the middle of the code interrupts the natural flow of the code (from a human readability standpoint).

It is good coding style to explicitly use the return keyword and the ending semicolon:

sub rand_elem {
    return $_[rand @_];
}

Naming

It would be better to use longer, more meaningful names for variables $n and $k.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.