Simple Encryption for Strings

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
7
down vote

favorite
1












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';







share|improve this question



























    up vote
    7
    down vote

    favorite
    1












    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';







    share|improve this question























      up vote
      7
      down vote

      favorite
      1









      up vote
      7
      down vote

      favorite
      1






      1





      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';







      share|improve this 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';









      share|improve this question












      share|improve this question




      share|improve this question








      edited Jul 10 at 20:13









      200_success

      123k14143399




      123k14143399









      asked Jul 10 at 15:20









      coder

      911425




      911425




















          3 Answers
          3






          active

          oldest

          votes

















          up vote
          9
          down vote



          accepted










          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)
          : mapmake_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) 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';






          share|improve this answer






























            up vote
            8
            down vote













            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.






            share|improve this answer























            • Yes, a simple table makes much more sense than a map - I didn't see the wood for the trees! Oh, and I like the use of std::rotate() - that's an <algorithm> I'd almost forgotten exists. (But note that it only works for character codings with contiguous letters - certainly not EBCDIC, nor for Latin-1 if we want to support alphabets such as Swedish).
              – Toby Speight
              Jul 11 at 7:21


















            up vote
            6
            down vote













            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) : datainput
            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);
            ;





            share|improve this answer























            • +1 for suggesting a mathematical approach for plain ASCII.
              – Edward
              Jul 11 at 1:29










            Your Answer




            StackExchange.ifUsing("editor", function ()
            return StackExchange.using("mathjaxEditing", function ()
            StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
            StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
            );
            );
            , "mathjax-editing");

            StackExchange.ifUsing("editor", function ()
            StackExchange.using("externalEditor", function ()
            StackExchange.using("snippets", function ()
            StackExchange.snippets.init();
            );
            );
            , "code-snippets");

            StackExchange.ready(function()
            var channelOptions =
            tags: "".split(" "),
            id: "196"
            ;
            initTagRenderer("".split(" "), "".split(" "), channelOptions);

            StackExchange.using("externalEditor", function()
            // Have to fire editor after snippets, if snippets enabled
            if (StackExchange.settings.snippets.snippetsEnabled)
            StackExchange.using("snippets", function()
            createEditor();
            );

            else
            createEditor();

            );

            function createEditor()
            StackExchange.prepareEditor(
            heartbeatType: 'answer',
            convertImagesToLinks: false,
            noModals: false,
            showLowRepImageUploadWarning: true,
            reputationToPostImages: null,
            bindNavPrevention: true,
            postfix: "",
            onDemand: true,
            discardSelector: ".discard-answer"
            ,immediatelyShowMarkdownHelp:true
            );



            );








             

            draft saved


            draft discarded


















            StackExchange.ready(
            function ()
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f198226%2fsimple-encryption-for-strings%23new-answer', 'question_page');

            );

            Post as a guest






























            3 Answers
            3






            active

            oldest

            votes








            3 Answers
            3






            active

            oldest

            votes









            active

            oldest

            votes






            active

            oldest

            votes








            up vote
            9
            down vote



            accepted










            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)
            : mapmake_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) 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';






            share|improve this answer



























              up vote
              9
              down vote



              accepted










              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)
              : mapmake_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) 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';






              share|improve this answer

























                up vote
                9
                down vote



                accepted







                up vote
                9
                down vote



                accepted






                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)
                : mapmake_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) 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';






                share|improve this answer















                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)
                : mapmake_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) 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';







                share|improve this answer















                share|improve this answer



                share|improve this answer








                edited Jul 10 at 16:58


























                answered Jul 10 at 16:39









                Toby Speight

                17.2k13486




                17.2k13486






















                    up vote
                    8
                    down vote













                    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.






                    share|improve this answer























                    • Yes, a simple table makes much more sense than a map - I didn't see the wood for the trees! Oh, and I like the use of std::rotate() - that's an <algorithm> I'd almost forgotten exists. (But note that it only works for character codings with contiguous letters - certainly not EBCDIC, nor for Latin-1 if we want to support alphabets such as Swedish).
                      – Toby Speight
                      Jul 11 at 7:21















                    up vote
                    8
                    down vote













                    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.






                    share|improve this answer























                    • Yes, a simple table makes much more sense than a map - I didn't see the wood for the trees! Oh, and I like the use of std::rotate() - that's an <algorithm> I'd almost forgotten exists. (But note that it only works for character codings with contiguous letters - certainly not EBCDIC, nor for Latin-1 if we want to support alphabets such as Swedish).
                      – Toby Speight
                      Jul 11 at 7:21













                    up vote
                    8
                    down vote










                    up vote
                    8
                    down vote









                    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.






                    share|improve this answer















                    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.







                    share|improve this answer















                    share|improve this answer



                    share|improve this answer








                    edited Jul 10 at 21:10


























                    answered Jul 10 at 20:48









                    Jerry Coffin

                    27.3k360123




                    27.3k360123











                    • Yes, a simple table makes much more sense than a map - I didn't see the wood for the trees! Oh, and I like the use of std::rotate() - that's an <algorithm> I'd almost forgotten exists. (But note that it only works for character codings with contiguous letters - certainly not EBCDIC, nor for Latin-1 if we want to support alphabets such as Swedish).
                      – Toby Speight
                      Jul 11 at 7:21

















                    • Yes, a simple table makes much more sense than a map - I didn't see the wood for the trees! Oh, and I like the use of std::rotate() - that's an <algorithm> I'd almost forgotten exists. (But note that it only works for character codings with contiguous letters - certainly not EBCDIC, nor for Latin-1 if we want to support alphabets such as Swedish).
                      – Toby Speight
                      Jul 11 at 7:21
















                    Yes, a simple table makes much more sense than a map - I didn't see the wood for the trees! Oh, and I like the use of std::rotate() - that's an <algorithm> I'd almost forgotten exists. (But note that it only works for character codings with contiguous letters - certainly not EBCDIC, nor for Latin-1 if we want to support alphabets such as Swedish).
                    – Toby Speight
                    Jul 11 at 7:21





                    Yes, a simple table makes much more sense than a map - I didn't see the wood for the trees! Oh, and I like the use of std::rotate() - that's an <algorithm> I'd almost forgotten exists. (But note that it only works for character codings with contiguous letters - certainly not EBCDIC, nor for Latin-1 if we want to support alphabets such as Swedish).
                    – Toby Speight
                    Jul 11 at 7:21











                    up vote
                    6
                    down vote













                    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) : datainput
                    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);
                    ;





                    share|improve this answer























                    • +1 for suggesting a mathematical approach for plain ASCII.
                      – Edward
                      Jul 11 at 1:29














                    up vote
                    6
                    down vote













                    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) : datainput
                    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);
                    ;





                    share|improve this answer























                    • +1 for suggesting a mathematical approach for plain ASCII.
                      – Edward
                      Jul 11 at 1:29












                    up vote
                    6
                    down vote










                    up vote
                    6
                    down vote









                    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) : datainput
                    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);
                    ;





                    share|improve this answer















                    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) : datainput
                    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);
                    ;






                    share|improve this answer















                    share|improve this answer



                    share|improve this answer








                    edited Jul 11 at 0:25


























                    answered Jul 10 at 21:22









                    Snowhawk

                    3,9901925




                    3,9901925











                    • +1 for suggesting a mathematical approach for plain ASCII.
                      – Edward
                      Jul 11 at 1:29
















                    • +1 for suggesting a mathematical approach for plain ASCII.
                      – Edward
                      Jul 11 at 1:29















                    +1 for suggesting a mathematical approach for plain ASCII.
                    – Edward
                    Jul 11 at 1:29




                    +1 for suggesting a mathematical approach for plain ASCII.
                    – Edward
                    Jul 11 at 1:29












                     

                    draft saved


                    draft discarded


























                     


                    draft saved


                    draft discarded














                    StackExchange.ready(
                    function ()
                    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f198226%2fsimple-encryption-for-strings%23new-answer', 'question_page');

                    );

                    Post as a guest













































































                    Popular posts from this blog

                    Chat program with C++ and SFML

                    Function to Return a JSON Like Objects Using VBA Collections and Arrays

                    Will my employers contract hold up in court?