Letter frequency analysis (Breaking Vigenere) I'm trying to improve my Perl and am doing some wargames online. I wrote a small utility to help me pass a stage.


  The main weakness of a simple substitution cipher is repeated use of a simple key. In the previous exercise you were able to introduce arbitrary plaintext to expose the key. In this example, the cipher mechanism is not available to you, the attacker.
  
  However, you have been lucky. You have intercepted more than one message. The password to the next level is found in the file ‘krypton4’. You have also found 3 other files. (found1, found2, found3)
  
  You know the following important details
  
  
  The message plaintexts are in English 
  They were produced from the same key
  The cyphertexts are UPPERCASE
  The ciphertexts in blocks of 5
  




#!/usr/bin/perl

use strict;
use warnings;
use feature 'say';


use File::Slurp;


my @FILENAMES = ("found1", "found2", "found3");
my @LETTERS = ("A" .. "Z");
my @ENGLISH_FREQ = ("E", "T", "A", "O", "I", "N", "S", 
                    "H", "R", "D", "L", "C", "U", "M", 
                    "W", "F", "G", "Y", "P", "B", "V", 
                    "K", "J", "X", "Q", "Z");

my $ENCRYPTED_STRING = read_file("krypton4");
$ENCRYPTED_STRING =~ s/\s+//g;


sub count_frequency {
    my ($string) = @_;
    my %occurences = count_occurence($string);
    return map { $_ =&gt; $occurences{$_} / length($string) } keys %occurences;
}

sub count_occurence {
    my ($string) = @_;
    my %frequency = map { $_ =&gt; 0 } @LETTERS;
    foreach (split //, $string) {
        $frequency{$_}++;
    }
    return %frequency;
}

sub get_single_key {
    my (%text_freq) = @_;
    my @text = ();
    foreach my $name ( sort { $text_freq{$b} &lt;=&gt; $text_freq{$a} } keys %text_freq ) {
        push @text, $name;
    }
    return map { $text[$_] =&gt; $ENGLISH_FREQ[$_] } (0 .. $#text);
}

foreach (@FILENAMES) {
    # Read the text
    my $text = read_file($_);
    $text =~ s/\s+//g;

    # Perform Letter Frequency Analysis
    my %cypher_key = get_single_key(count_frequency($text));

    # Try to decrypt the text
    foreach (split //, $ENCRYPTED_STRING) {
        print "$cypher_key{$_}";
    }
    say "";
}


It was some puzzling afterwards, but it did its job.
use File::Slurp;



File::Slurp is considered broken by a lot of people. A good alternative is Path::Tiny.

It's also good practice to import only what you need when you use a module. That helps others to see where a function is coming from (like your read_file, which I tried to find in your code), and it helps to keep your namespace clean.

use Path::Tiny 'path';

my $text = path('krypton4')-&gt;slurp;





my @FILENAMES = ("found1", "found2", "found3");



All caps names are typically used for constants only. You can create a constant with the use constants pragma. That works by installing a sub into your namespace that returns a scalar. As a function, there's no sigil, and you need to use references for arrays.

The qw() quoted words operator is a nice shrotand for creating lists of quoted words, as you don't have to bother with commas and quotes.

use constant FILENAMES =&gt; [ qw(found1 found2 found3) ];





my $ENCRYPTED_STRING = read_file("krypton4");
$ENCRYPTED_STRING =~ s/\s+//g;



You only use this variable once. It's not really configuration either, so making it a constant feels wrong. In Perl, we try to declare variables as close to where they are used as possible, and in the smallest scope possible. I would recommend simply using Path::Tiny's path down in your foreach loop.

You could however add a constant for the krypton4 file name if you wanted.

use constant ENCRYPTED_FILE =&gt; 'krypton4';


If your Perl is at least 5.14, you can use the /r modifier on regex substitution to return an altered version, instead of doing in-place edit. 

foreach my $item (split //, path(ENCRYPTED_FILE)-&gt;slurp =~ s/\s+//gr) { ... }


If your Perl is older, you can also use map to do that in one line. Note that s///g returns the number of substitutions, you have to explicitly return $_ from the block to get the text.

foreach my $item (split //, map { s/\s+//g; $_ } path(ENCRYPTED_FILE)-&gt;slurp) { ... }





foreach (split //, $string) {
    $frequency{$_}++;
}



If you use $_ explicitly in a loop body, it's clearer to use a named variable instead. I would call this $letter.

foreach my $letter (split //, $string) {
    $frequency{$letter}++;
}





sub get_single_key {
    my (%text_freq) = @_;
    my @text = ();
    foreach my $name ( sort { $text_freq{$b} &lt;=&gt; $text_freq{$a} } keys %text_freq ) {
        push @text, $name;
    }

    return map { $text[uc($_)] =&gt; $ENGLISH_FREQ[$_] } (0 .. $#text);
}



The frequency you've calculated with count_frequency is the frequency of each letter in the text. Your variable %text_frequency should thus be named %letter_frequency. But that's long. Why not stick with %frequency? It worked above.

You don't need to initialize arrays and hashes with an empty list. Perl does that for you.

I didn't read the algorithm description, so I don't know exactly what this does, but @text feels like the name is wrong. That variable is not an array of texts, or ordered pieces of one text. I think it contains the letters ordered by frequency. So @letters might be better.

It's good that you used a named loop variable, but again, this is a letter, right? So $letter is way more fitting than $name. 

You can just assign the list of keys that sort returns to @letters.

There's a bug in your map statement. The $_ is not the letter, but the index. Your iteration 0 .. $#text gives you all the indexes. You are ucing the index, not the value. 

You don't need the parentheses around the range. map takes a list, and the range operator .. gives you a list.

sub get_single_key {
    my (%frequency) = @_;

    my @letters = sort { $frequency{$b} &lt;=&gt; $frequency{$a} } keys %frequency;

    return map { uc $letters[$_] =&gt; ENGLISH_FREQUENCIES-&gt;[0] } 0 .. $#letters;
}


Since you have that comment Perform Letter Frequency Analysis where you use this function, and you return a hash, which implies a bunch of things, the name get_single_key might not be optimal.




foreach (split //, $ENCRYPTED_STRING) {
    print "$cypher_key{$_}";
}



A postfix foreach (or for for brevity) will be easier to read here.

You don't have to quote a single variable. There is no need for interpolation.

print $cypher_key{$_} 
    for split //, path(ENCRYPTED_FILE)-&gt;slurp =~ s/\s+//gr;





say "";



It's shorter than print "\n" (or print "$/" if you insist on being portable), but it just feels off. It's good though that you realized that you need an empty string so it does not output $_ instead.