Simple Affine cipher

Question

The affine cipher is a simple mathematical substitution cipher. If you're interested in the details behind how it works, this page goes further into detail.

After writing a program to encrypt and decrypt text using the affine cipher, I felt that it seemed needlessly cluttered. I have also noticed that there are no questions related to the affine cipher on Code Review, at least not for C++.

Here's the code:

#include <iostream>
#include <algorithm>
#include <numeric>
#include <cmath>

int gcd(int a, int b);
int modInverse(int a, int b); // from https://rosettacode.org/wiki/Modular_inverse#C.2B.2B

int main(){
    std::string choice;
    do{
        std::cout << "Encrypt or Decrypt? [e/d] = ";
        std::getline(std::cin, choice);
        std::transform(choice.begin(), choice.end(), choice.begin(), ::tolower);
    } while(choice.length() > 1 || choice != "e" && choice != "d");

    std::cout << "\nInput string: ";
    std::string input;
    std::getline(std::cin, input);

    int a, b;
    do{
        std::cout << "\na and b must be coprime\na = ";
        std::cin >> a;
        std::cout << "b = ";
        std::cin >> b;
    } while(std::cin.fail() || gcd(a,b) != 1);

    std::cout << '\n';

    if(choice == "e"){
        for(int i = 0; i < input.length(); ++i){
            if(input[i] >= 'a' && input[i] <= 'z'){
                std::cout << (char)((a * (input[i] - 'a') + b) % 26 + 'a');
            } else if(input[i] >= 'A' && input[i] <= 'Z'){
                std::cout << (char)((a * (input[i] - 'A') + b) % 26 + 'A');
            } else{
                std::cout << input[i];
            }
        }
    } else{
        for(int i = 0; i < input.length(); ++i){
            if(input[i] >= 'a' && input[i] <= 'z'){
                std::cout << (char)(modInverse(a, 26) * (26 + input[i] - 'a' - b) % 26 + 'a');
            } else if(input[i] >= 'A' && input[i] <= 'Z'){
                std::cout << (char)(modInverse(a, 26) * (26 + input[i] - 'A' - b) % 26 + 'A');
            } else{
                std::cout << input[i];
            }
        }
    }

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

int gcd(int a, int b){
    return b == 0 ? a : gcd(b, a % b);
}

int modInverse(int a, int b){
    int b0 = b, t, q;
    int x0 = 0, x1 = 1;
    if (b == 1) return 1;
    while (a > 1) {
        q = a / b;
        t = b, b = a % b, a = t;
        t = x0, x0 = x1 - q * x0, x1 = t;
    }
    if (x1 < 0) x1 += b0;
    return x1;
}

Here is an example output for the encryption side:

Encrypt or Decrypt? [e/d] = e

Input string: Hello World!

a and b must be coprime
a = 5
b = 9

Sdmmb Pbqmy!

And here is an example output for the decryption side:

Encrypt or Decrypt? [e/d] = d

Input string: Sdmmb Pbqmy!

a and b must be coprime
a = 5
b = 9

Hello World!

I am using the modInverse() function from Rosetta Code. This is quasi-related to my ongoing series of classical ciphers. So far I have done a simple Caesar cipher and an Atbash cipher.

How can I improve this code, both for readability and for efficiency?

Answer 1

It's very easy to understand each step in your program, even without understanding the actual algorithm you're implementing, which is really nice. Here's how I would improve things.

Use Functions

Each part of your main() function looks to me like it should be a separate named function. For example, it's clear that you're asking for whether the user wants to encrypt or decrypt, then getting the input string, then asking for a and b, then doing the encryption or decryption. Each of those things is a separate task and as such should be its own function. Your main() should look more like this:

int main() {
    std::string choice = getTaskFromUser(); // returns either 'e' or 'd'
    std::string input = getInputFromUser(); // return string to encrypt or decrypt
    int a, b;
    getAAndB(a, b); // Gets a and b
    if (choice == TASK_ENCRYPT)
    {
        displayEncryptedText(input, a, b);
    }
    else
    {
        displayDecryptedText(input, a, b);
    }

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

Usability

There are a few things in your program that are named confusingly. First, I had to look up what "coprime" meant. It's not a hard concept, but I'd never heard it before. It might be worth printing out a one or two-sentence description of what it means when asking the user to input coprime numbers.

The variables a and b may also be poorly named. What function do they serve in the algorithm? Looking at the linked page, it looks like the cypher consists of a linear equation of the form y = ax + b where a is the slope and b is the intercept. That might be a good set of names, or it might not depending on how a typical user of such an algorithm thinks about it. a and b aren't terrible given the abstractness of the algorithm, but if you can improve those names, I recommend it. Certainly, as a user, I wouldn't understand what they represent.

Avoid Magic Numbers

Looking at the code, I see this mysterious number "26" repeated in several places. What does it represent? As an English speaker, I can guess that it's the number of letters in the alphabet, but what if I want to include numbers and symbols? What if I want to implement this algorithm in a different language with more or fewer letters? At the very least, there should be a named constant for the value 26. I recommend something like:

const int kAlphabetSize = 26;

That way you can change it later. Also, the magic values a, A, z, and Z should be handled better. Do you want to preserve the case of the original text? It seems to me like it makes the cypher weaker if you do. It's additional context that someone trying to decrypt it can use. I recommend allowing at least all the (printable) ASCII characters to be used as one large alphabet. It simplifies the code as you don't end up special-casing as many things.