Short and Messy Polybius Square

Question

I attempted to recreate the Polybius square, also called the Polybius checkerboard, which was used in Ancient Greece for cryptography. Since it is an uncommon cipher, it is nowhere on Code Review.

Although the program technically works, it ended up very messy.

#include <iostream>
#include <algorithm>
#include <vector>
#include <sstream>
#include <iterator>
#include <unordered_map>

std::string getChoice();
std::vector<std::string> getCoords();

int main(){
    std::string choice = getChoice();

    if(choice == "e"){
        std::cout << "Input string: ";
        std::string input;
        std::getline(std::cin, input);
        input.erase(remove_if(input.begin(), input.end(), isspace), input.end());

        std::unordered_map<char, unsigned int> alphaToInt = {
            {'A', 11},
            {'B', 12},
            {'C', 13},
            {'D', 14},
            {'E', 15},
            {'F', 21},
            {'G', 22},
            {'H', 23},
            {'I', 24},
            {'J', 24}, // same as I
            {'K', 25},
            {'L', 31},
            {'M', 32},
            {'N', 33},
            {'O', 34},
            {'P', 35},
            {'Q', 41},
            {'R', 42},
            {'S', 43},
            {'T', 44},
            {'U', 45},
            {'V', 51},
            {'W', 52},
            {'X', 53},
            {'Y', 54},
            {'Z', 55},
        };

        for(int i = 0; i < input.length(); ++i){
            std::cout << alphaToInt[input[i]] << ' ';
        }
    } else{
        std::vector<std::string> coords = getCoords();
        std::string output;

        const std::string squareIJ[5][5] = {
            {"A", "B", "C", "D", "E"},
            {"F", "G", "H", "I", "K"},
            {"L", "M", "N", "O", "P"},
            {"Q", "R", "S", "T", "U"},
            {"V", "W", "X", "Y", "Z"},
        };

        for(int i = 0; i < coords.size(); ++i){
            std::string coord = coords[i];
            output.append(squareIJ[coord[0] - '1'][coord[1] - '1']);
        }

        std::cout << output;

    }

    std::cout << '\n';
    return 0;
}

std::string getChoice(){
    std::string choice;
    do{
        std::cout << "Encrypt or decrypt [e/d] = ";
        std::getline(std::cin, choice);
        if(choice == "E") choice = "e";
        std::transform(choice.begin(), choice.end(), choice.begin(), ::tolower);
    } while(choice != "e" && choice != "d");

    return choice;
}

std::vector<std::string> getCoords(){
    std::cout << "Coordinates (separate with spaces): ";
    std::string strCoords;
    std::getline(std::cin, strCoords);
    std::stringstream streamCoords(strCoords);
    std::istream_iterator<std::string> begin(streamCoords);
    std::istream_iterator<std::string> end;
    std::vector<std::string> coords(begin, end);
    return coords;
}

The Polybius square is a simple way to assign characters numbers, and then "encrypt" and "decrypt" based off of those numbers. As shown in the unordered map and char array, the numbers correspond to the row and column on a 5x5 square:

The getChoice() function simply gets the e or d character to choose encryption or decryption.

The getCoords() function gets a string, and then tokenizes the string into a vector of strings, each corresponding to the coordinates of a character.

Example of "encryption":

Encrypt or decrypt [e/d] = e
Input string: BAT
12 11 44

Example of "decryption":

Encrypt or decrypt [e/d] = d
Coordinates (separate with spaces): 12 11 44
BAT

This code, in my opinion, is quite messy. How can I improve it?

Answer 1

It's not that messy IMO, but there are a few things which you could have done better:

• Always compile code with every warning turned on, and fix them. When compiled with -Wall, your code produces 2 same warnings:

  warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  

  You should fix them.

• Your choice variable really wants to be an enum instance:

  enum class mode {
      encrypt, decrypt
  };
  

• You have some redundant code:

  if(choice == "E") choice = "e";
  // redundant if in the next line your lower casing every character in 'choice'
  

• const correctness: alphaToInt can and should be const, as you're not going to modify it no matter what.

• getCoords can be reduced considerably (it looks too bloated otherwise IMO):

  std::vector<std::string> getCoords(){
      std::cout << "Coordinates (separate with spaces): ";
      std::string strCoords;
      std::getline(std::cin, strCoords);
  
      std::stringstream streamCoords(strCoords);
      return{ std::istream_iterator<std::string>{ streamCoords }, std::istream_iterator<std::string>{} };
  } 
  

• squareIJ doesn't have to be a std::string[5][5], it can be a simple char[5][5]. Don't allocate memory if you don't have too (this might actually not allocate memory for the std::string, due to SSO actually).

• Use auto to simply some variable definitions if you want to:

  // std::vector<std::string> coords = getCoords();
  auto coords = getCoords();
  
  // ...
  

• Use ranged-based loop instead of index loop. For example,

  for(int i = 0; i < coords.size(); ++i){
      std::string coord = coords[i];
      output.push_back(squareIJ[coord[0] - '1'][coord[1] - '1']);
  }
  

  becomes

  for (const auto& coord : coords)
      output.push_back(squareIJ[coord[0] - '1'][coord[1] - '1']);
  

• Include every necessary headers, you forgot <cctype> for std::isspace.

• If you expect every character to be upper case, either (1) tell the user (not recommended) or (2) upper case the string yourself:

  std::transform(input.begin(), input.end(), input.begin(), ::toupper);
  

Answer 2

I think I'd start by splitting it into two pieces, one to encrypt and one to decrypt. As a user, I'd much rather type decrypt [whatever] or encrypt [whatever] than have to interactively enter an 'e' or 'd' to tell it whether to encrypt or decrypt.

Likewise, each would act as a filter, taking its input from cin, and writing its output to cout (or possibly read/write other files if you specify them on the command line). Again, as a user I'd much rather be able to take input from a file and/or write output to a file (and this still allows me to run it interactively in the rare circumstance that I really want to).

For implementation of the cipher, I'd consider using a little math instead of the tables you're currently using. For example, encrypting would look something like this:

int encrypt(char ch) { 
    int linear = ch - 'A' - (ch > 'I');
    int row = linear / 5 + 1;
    int column = linear % 5 + 1;
    return row * 10 + column;
}

Using this, a complete encryption program could look something like this:

int main() {
    int ch;
    while (EOF != (ch = getchar()))
        if (std::isupper((unsigned char)ch))
            std::cout << encrypt(ch) << ' ';
}

Likewise, decryption might look something on this order:

char decrypt(int coords) {
    int column = coords % 10 - 1;
    int row = coords / 10 - 1;
    int linear = row * 5 + column + 'A';
    if (linear > 'I')
        ++linear;

    return linear;
}

The decryption program using this would look something like this:

int main() {
    int ch;
    while (std::cin >> ch)
        std::cout << decrypt(ch);
}

I suppose some might take issue with: int linear = ch - 'A' - (ch > 'I'); Personally, I find it perfectly understandable, but some might prefer code more like:

int linear = ch - 'A';
if (ch > 'I')
    --linear;

The other point to consider is that in theory this is marginally less portable. In particular, it assumes that upper case characters are assigned a contiguous range of character codes, so 'B' = 'A' + 1, and so on throughout the alphabet. Although that's true with all sane character sets, there's one (EBCDIC) that actually breaks the alphabet into three pieces, and inserts some special characters between those pieces. At least in my opinion, however, this is sufficiently stupid that it deserves only ridicule. Catering your code to it would (in my opinion) be relatively silly as a rule.