Simple Encryption for Strings
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
7
down vote
favorite
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';
c++ c++11 caesar-cipher
add a comment |Â
up vote
7
down vote
favorite
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';
c++ c++11 caesar-cipher
add a comment |Â
up vote
7
down vote
favorite
up vote
7
down vote
favorite
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';
c++ c++11 caesar-cipher
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';
c++ c++11 caesar-cipher
edited Jul 10 at 20:13
200_success
123k14143399
123k14143399
asked Jul 10 at 15:20
coder
911425
911425
add a comment |Â
add a comment |Â
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';
add a comment |Â
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.
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 ofstd::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
add a comment |Â
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);
;
+1 for suggesting a mathematical approach for plain ASCII.
â Edward
Jul 11 at 1:29
add a comment |Â
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';
add a comment |Â
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';
add a comment |Â
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';
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';
edited Jul 10 at 16:58
answered Jul 10 at 16:39
Toby Speight
17.2k13486
17.2k13486
add a comment |Â
add a comment |Â
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.
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 ofstd::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
add a comment |Â
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.
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 ofstd::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
add a comment |Â
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.
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.
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 ofstd::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
add a comment |Â
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 ofstd::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
add a comment |Â
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);
;
+1 for suggesting a mathematical approach for plain ASCII.
â Edward
Jul 11 at 1:29
add a comment |Â
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);
;
+1 for suggesting a mathematical approach for plain ASCII.
â Edward
Jul 11 at 1:29
add a comment |Â
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);
;
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);
;
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
add a comment |Â
+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
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
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
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password