# Substitution Cipher: Which One?

I'm a beginner and I have a question (somehow silly and stupid :) ). Today I decided to challenge myself and I came around the challenge that wanted me to create a program that ciphers (or encrypts) the message using the substitution cipher method. I solved the challenge by myself but mine is way different than the solution itself. I just want to know which one is better and why? And also is there anything I missed in my own code?

So here is the code I've written:

#include <iostream>
#include <string>
using namespace std;
int main()
{
string secretMessage {};
string alphabet {"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"};
string key {"XZNLWEBGJHQDYVTKFUOMPCIASRxznlwebgjhqdyvtkfuompciasr"};
cout << "Enter your secret message: ";
getline(cin, secretMessage);
//Encryption
for(size_t i{0}; i<secretMessage.length(); ++i){
for(size_t j{0}; j<alphabet.length(); ++j){
if (secretMessage.at(i) == alphabet.at(j)){
secretMessage.at(i) = key.at(j);
break;
}
}
}
cout << "Encrypting The Message..." << endl;
cout << "Encrypted Message: " << secretMessage << endl;
//Decryption
for(size_t i{0}; i<secretMessage.length(); ++i){
for(size_t j{0}; j<key.length(); ++j){
if (secretMessage.at(i) == key.at(j)){
secretMessage.at(i) = alphabet.at(j);
break;
}
}
}
cout << "\nDecrypting The Encryption..." << endl;
cout << "Decrypted: " << secretMessage << endl;
return 0;
}


And here is the solution:

#include <iostream>
#include <string>
using namespace std;
int main()
{
string secretMessage {};
string alphabet {"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"};
string key {"XZNLWEBGJHQDYVTKFUOMPCIASRxznlwebgjhqdyvtkfuompciasr"};
string encryptedMessage {};
string decryptedMessage {};
cout << "Enter your secret message: ";
getline(cin, secretMessage);
cout << "\nEncrypting Message..." << endl;
//Encryption
for(char c:secretMessage){
size_t position = alphabet.find(c);
if (position != string::npos){
char newChar {key.at(position)};
encryptedMessage += newChar;
} else{
encryptedMessage += c;
}
}
cout << "Encrypted Message: " << encryptedMessage << endl;
//Decryption
cout << "\nDecrypting Message..." << endl;
for(char c:encryptedMessage){
size_t position = key.find(c);
if (position != string::npos){
char newChar {alphabet.at(position)};
decryptedMessage += newChar;
} else{
decryptedMessage += c;
}
}
cout << "Decrypted Message: " << decryptedMessage << endl;
return 0;
}


Note:I have also included the decryption part too

• I've edited to be clearer which code is yours and which is not (the latter isn't reviewable here). Please make sure that you are allowed to re-publish the solution code on Stack Exchange - I don't want you to be in trouble! – Toby Speight Feb 3 '20 at 17:58

# Avoid using namespace std

It's a bad idea to import large namespaces such as std into the global namespace, and it could possibly even change the meaning of your code when a later standard adds new identifiers. Reserve using namespace for those few namespaces specifically designed to be used that way (notably std::literals).

# Use const appropriately

We don't want alphabet or key to be modified, so prevent accidents by declaring them as const std::string.

# getline() can fail

If the input stream is closed, getline() will read nothing, and the string will be empty. That's probably acceptable for this program, but probably worth a comment to show that you've considered this.

# Use range-based for

We have an integer loop whose value is used only for indexing into the string:

for(size_t i{0}; i<secretMessage.length(); ++i){
// use secretMessage[i] here


That's easier to read with modern range-based syntax:

for (auto const c: secretMessage) {
// use c here


# Use standard algorithms

We can include <algorithm> to get some really useful functions from the standard library. In particular, std::find() could replace the loop that searches in the alphabet.

# Reduce duplication

Notice that there are two blocks of code that are almost identical, due to the symmetric nature of this cipher. The only difference is that the role of alphabet and key are swapped. This makes it a good candidate to extract as a function:

std::string substitute(const std::string& message,
const std::string& from,
const std::string& to)


Then we could just call it:

auto encrypted = substitute(secretMessage, alphabet, key);
auto decrypted = substitute(encrypted, key, alphabet);


# Avoid linear search

We can be more efficient with our lookup than simple linear search. For example, we could construct a pair of std::map<char,char> from the alphabet and key.

# Clean up the plaintext

I know this is just toy encryption, but for real code, we'd want to overwrite the decrypted text when we finish using it, to reduce the ability for an attacker to access it (e.g. from a core dump). Remembering <algorithm>, we'd use std::fill() for this.

# Omit the return

In main() (and only there), we can omit the return statement and the compiler will automatically return 0 for us. It's a fairly common convention to omit the return if we don't have any non-zero (i.e. error) returns from main().

• Thank you for the time you put and your help...I appreciate it and to be honest I've never seen anyone like you explain things completely :)...So thank you... – David Peterson Feb 3 '20 at 18:44
• Is std::fill guaranteed to survive the compiler optimizations? Otherwise your advice wouldn't make sense. – Roland Illig Feb 3 '20 at 22:10
• Good question @Roland, and I don't know the answer right now. Security code is always harder than one expects! – Toby Speight Feb 4 '20 at 8:16