Simple Encryption for Strings

Question

I have written this code for practice. Here we will enter the number by which we want to shift alphabets. For eg.

Enter by how many numbers you want to shift alphabets: 4

Enter string: Programmer C++

Encrypted String: Lnkcnwiian Y++

Decryted String: Programmer C++

Review this code and suggest better C++11 and C++14 alternatives.

#include <iostream>
#include <string>
#include <vector>
#include <unordered_map>

class Cipher
{
    unsigned n;
    const std::vector<char> alphabetL { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H',
      'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W',
      'X', 'Y', 'Z' };
    const std::vector<char> alphabetS { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h',
      'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w',
      'x', 'y', 'z' };
    std::unordered_map<char, char> shiftByNL;
    std::unordered_map<char, char> shiftByNS;

  public:
    Cipher(unsigned n = 0);
    std::string encrypt(std::string&);
    std::string decrypt(std::string&);

  private:
    char byValue(const char); //Find by value
};

Cipher::Cipher(unsigned n)
{
    for (int i = 0; i < n; i++)
    {
        shiftByNL.insert(std::make_pair(alphabetL[i], alphabetL[(26 - n) + i]));
        shiftByNS.insert(std::make_pair(alphabetS[i], alphabetS[(26 - n) + i]));
    }
    for (int i = n; i < 26; i++)
    {
        shiftByNL.insert(std::make_pair(alphabetL[i], alphabetL[i - n]));
        shiftByNS.insert(std::make_pair(alphabetS[i], alphabetS[i - n]));
    }
}

char Cipher::byValue(const char value)
{
    for (auto itL = shiftByNL.begin(), itS = shiftByNS.begin();
          itL != shiftByNL.end(), itS != shiftByNS.end(); ++itL, ++itS)
    {
        if (itL->second == value)
        {
            return itL->first;
        }
        else if (itS->second == value)
        {
            return itS->first;
        }
    }
    //not an alphabet
    return value;
}

std::string Cipher::encrypt(std::string& original)
{
    std::string encrypted;
    std::size_t len = original.length();
    for (std::size_t i = 0; i < len; i++)
    {
        if (original[i] == ' ')
        {
            encrypted = encrypted + ' ';
        }
        else if (shiftByNS.find(original[i]) != shiftByNS.end())
        {
            auto search = shiftByNS.find(original[i]);
            encrypted = encrypted + search->second;
        }
        else if (shiftByNL.find(original[i]) != shiftByNL.end())
        {
            auto search = shiftByNL.find(original[i]);
            encrypted = encrypted + search->second;
        }
        else //not an alphabet
        {
            encrypted = encrypted + original[i];
        }
    }
    return encrypted;
}

std::string Cipher::decrypt(std::string& encrypted)
{
    std::string decrypted;
    std::size_t len = encrypted.length();
    for (std::size_t i = 0; i < len; i++)
    {
        if (encrypted[i] == ' ')
        {
            decrypted = decrypted + ' ';
        }
        else
        {
            char key = byValue(encrypted[i]);
            decrypted = decrypted + key;
        }
    }
    return decrypted;
}

int main()
{
    unsigned num;
    std::cout << "Enter by how many numbers you want to shift alphabets: ";
    std::cin >> num;
    std::cin.ignore();
    Cipher cipher(num);
    std::string str;
    std::cout << "Enter string: ";
    std::getline(std::cin, str);
    std::string res = cipher.encrypt(str);
    std::cout << "Encrypted String: ";
    std::cout << res << '\n';
    std::cout << "Decryted String: ";
    std::string decrypted = cipher.decrypt(res);
    std::cout << decrypted << '\n';
}

Answer 1

I don't think you should hard-code the length of alphabet as 26 here:

for (int i = n; i < 26; i++)

Doing so will make it harder to adapt this code to languages with a different number of letters. Instead, prefer to use alphabetL.size() and alphabetS.size() (and probably use separate loops, as some languages don't have the same number of upper-case and lower-case letters).

---

It's probably advisable to test that n is less than the alphabet length (or to reduce it modulo length before using), and perhaps reject a zero shift (which is what we call a "weak key" for this cipher).

---

decrypt() is much slower than encrypt, because it does a search for every character. What you really want is a pair of maps, but not lowercase and uppercase - instead you want forward and reverse (with both upper and lower in the same map). In fact, I'd go further; allow negative values for the shift, and then we can create a separate Cipher object for the decryption.

---

Constructor that can accept a single argument should be explicit. I don't think a default of 0 is a good choice (see my comment about weak keys). Also, prefer initialization to assignment in constructors (GCC will warn, with -Weffc++).

---

Cipher::n is never initialized or used - we can drop that member. Also, the two constant vectors don't need to be members - they are used only in the constructor, where they can be shared between all instances. We can simplify further by making them string literals.

---

encrypt() and decrypt() both take a modifiable reference to their input, but it seems they should not modify the input string. You could pass a reference to const string, but I think it might be easier to accept a copy by value, and modify the copy.

Also, they shouldn't be modifying the Cipher object, so they can be declared const.

---

Modified code

Here's what I came up with:

#include <algorithm>
#include <unordered_map>

class Cipher
{
    const std::unordered_map<char, char> map;

public:
    explicit Cipher(int n);
    std::string encrypt(std::string) const;

private:
    static std::unordered_map<char, char> make_map(int n);
};


Cipher::Cipher(int n)
    : map{make_map(n)}
{}

std::unordered_map<char, char> Cipher::make_map(int n)
{
    // helper function to give a positive value for a%b
    auto mod = [](int a, int b){ a %= b; return a < 0 ? a + b : a; };

    std::unordered_map<char, char> map;
    {
        static const char alphabet[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
        int const m = sizeof alphabet - 1; // subtract the final NUL
        for (int i = 0;  i < m;  ++i) {
            map.insert(std::make_pair(alphabet[i], alphabet[mod(i+n,m)]));
        }
    }
    {
        static const char alphabet[] = "abcdefghijklmnopqrstuvwxyz";
        int const m = sizeof alphabet - 1; // subtract the final NUL
        for (int i = 0; i < m; i++) {
            map.insert(std::make_pair(alphabet[i], alphabet[mod(i+n,m)]));
        }
    }

    return map;
}

std::string Cipher::encrypt(std::string s) const
{
    std::transform(s.begin(), s.end(), s.begin(),
                   [=](char c){ auto it = map.find(c); return it == map.end() ? c : it->second; });
    return s;
}

I slightly modified the test program to allow it to accept command-line arguments (easier for test scripts):

#include <cstdlib>
#include <iostream>
int main(int argc, char **argv)
{
    int num;
    if (argc > 1) {
        char* pos;
        num = std::strtol(argv[1], &pos, 0);
        if (*pos || pos == argv[1]) {
            std::cerr << "Usage: " << argv[0] << " NUM WORD" << std::endl;
            return 1;
        }
    } else {
        std::cout << "Enter by how many numbers you want to shift alphabets: ";
        std::cin >> num;
        std::cin.ignore();
    }
    Cipher cipher(num);
    Cipher decipher(-num);

    std::string str;
    if (argc > 2) {
        str = argv[2];
    } else {
        std::cout << "Enter string: ";
        std::getline(std::cin, str);
    }

    std::string res = cipher.encrypt(str);
    std::string decrypted = decipher.encrypt(res);
    std::cout << "Encrypted String: "
              << res << '\n'
              << "Decrypted String: "
              << decrypted << '\n';
}

Answer 2

While I agree with the general idea of most of what Toby Speight says, for this case, I'd tend to avoid using a map, and instead just use a contiguous table, something on this general order:

#include <algorithm>
#include <string>
#include <iostream>
#include <array>
#include <limits>

class encrypt { 
    std::array<char, std::numeric_limits<unsigned char>::max()> table;
public:
    encrypt(int shift) { 
        std::iota(table.begin(), table.end(), 0);
        std::rotate(&table['A'], &table['Z'+1] - shift, &table['Z'+1]);
        std::rotate(&table['a'], &table['z'+1] - shift, &table['z'+1]);
    }

    char operator()(char in) const { return table[in]; }

    std::string &operator()(std::string &in) const {
        for (char &c : in)
            c = table[(unsigned char )c];
    }
};

class decrypt {
    std::array<char, std::numeric_limits<unsigned char>::max()> table;
public:
    decrypt(int shift) {        
        std::iota(table.begin(), table.end(), 0);        
        std::rotate(&table['A'], &table['A'] + shift, &table['Z'+1]);
        std::rotate(&table['a'], &table['a'] + shift, &table['z'+1]);
    }

    char operator()(char in) const { return table[in]; }

    std::string &operator()(std::string &in) const {
        for (char &c : in)
            c = table[(unsigned char)c];
    }
};

int main() { 
    std::string input = "Programmer C++";

    encrypt(4)(input);

    std::cout << input << "\n";

    decrypt(4)(input);

    std::cout << input << "\n";

}

As long as we're only dealing with a 256-character set, this seems perfectly reasonable (at least to me). If we wanted to deal with Unicode, Toby's would almost certainly be the preferred approach though.

Note: the main I've included is not intended as a suggestion of how main should work. It's really just enough to test that the rest of the code works as intended.

Answer 3

Cipher::Cipher(unsigned n)
{
    for (int i = 0; i < n; i++)
    {
        shiftByNL.insert(std::make_pair(alphabetL[i], alphabetL[(26 - n) + i]));
        shiftByNS.insert(std::make_pair(alphabetS[i], alphabetS[(26 - n) + i]));
    }
    for (int i = n; i < 26; i++)
    {
        shiftByNL.insert(std::make_pair(alphabetL[i], alphabetL[i - n]));
        shiftByNS.insert(std::make_pair(alphabetS[i], alphabetS[i - n]));
    }
}

The first loop has a signed-to-unsigned comparison.

Should the user be able to map the letter A to the letter B using -1? Does n have to be unsigned?

The user that calls std::vector<T>::operator[] needs to ensure the index is in range. What happens if n is larger than the alphabet size?

---

char Cipher::byValue(const char value)
{
    for (auto itL = shiftByNL.begin(), itS = shiftByNS.begin();
          itL != shiftByNL.end(), itS != shiftByNS.end(); ++itL, ++itS)
    {
        if (itL->second == value)
        {
            return itL->first;
        }
        else if (itS->second == value)
        {
            return itS->first;
        }
    }
    //not an alphabet
    return value;
}

Your search traverses both the upper and lower case maps. We know if value is upper or lower, so we can decide which container we want to search.

Know your algorithms! If you want to linearly search a container, use std::find/std::find_if. For the latter, you can use a predicate that searches member second of the pairs.

char Cipher::byValue(const char value)
{
    if (std::isupper(value)) {
        auto found = std::find_if(shiftByNL.begin(), shiftByNL.end(), mapped_second(value));
        return found != shiftByNL.end() ? found->first : value;
    }
    auto found = std::find(shiftByNS.begin(), shiftByNS.end(), mapped_second(value));
    return found != shiftByNS.end() ? found->first : value;
}

From here, you can DRY it up into a higher abstraction.

---

std::string Cipher::encrypt(std::string& original)
{
    std::string encrypted;
    std::size_t len = original.length();
    for (std::size_t i = 0; i < len; i++)
    {
        if (original[i] == ' ')
        {
            encrypted = encrypted + ' ';
        }
        else if (shiftByNS.find(original[i]) != shiftByNS.end())
        {
            auto search = shiftByNS.find(original[i]);
            encrypted = encrypted + search->second;
        }
        else if (shiftByNL.find(original[i]) != shiftByNL.end())
        {
            auto search = shiftByNL.find(original[i]);
            encrypted = encrypted + search->second;
        }
        else //not an alphabet
        {
            encrypted = encrypted + original[i];
        }
    }
    return encrypted;
}

original is not modified, so qualify it with const.

Consider reserving the encrypted string capacity.

Since you aren't using the index for anything besides container access, use the range-based for loop (or an algorithm like std::transform).

---

std::cin >> num;

If std::cin fails to read an unsigned value, the value that is assigned to num is 0. Check to make sure the value was correctly read from the stream.

---

Consider a policy based approach if you plan to support more than just ASCII. If you plan to stick with only ASCII, consider using a mathematical approach rather than multiple lookup tables.

Leverage the type system to provide information for the user. Phantom types can wrap its payload with tags like encrypted and unencrypted.

template <typename T>
struct string_data {
    explicit StringData(std::string const& input) : data{input} {}
    std::string data;
};

struct encrypted {};
struct unencrypted {};

class cipher {
    // ...
    string_data<encrypted> encrypt(string_data<unencrypted> const& data);
    string_data<unencrypted> decrypt(string_data<encrypted> const& data);
};