Implement Transposition cipher in c++ I try to implement the encryption by Columnar transposition.

Input: key_secret and Text_to_encrypt, both of them are in lower.

Output: encrypted_text.

The code works fine but I have some questions:


it warns: "Function 'begin' could not be resolved", Function 'end' could not be resolved, Function 'copy_if' could not be resolved. How can I get rid of it?
it creates mapOriginAlphabet slowly. How can I improve it?


I use gcc 5.3.0

Here the code: .h file

#ifndef TRANSPOSITIONCIPHER_H_
#define TRANSPOSITIONCIPHER_H_

#include &lt;iostream&gt;
#include &lt;string&gt;
#include &lt;algorithm&gt;
#include &lt;map&gt;
#include &lt;set&gt;

#define log(s) std::cout &lt;&lt; s &lt;&lt; std::endl

class transpositioncipher {
private:
    std::map&lt;char,bool&gt; mapOriginAlphabet;
    std::map&lt;char,char&gt; mapAlphabet;
    std::vector&lt;int&gt; vecOrder;
    std::string mKeySecret;

    std::string removeDuplicates(const std::string strOrigin);
    void createOrder();
    void createSubstitution();

public:
    transpositioncipher(std::string KeySecret);
    virtual ~transpositioncipher();

    std::string encrypt(const std::string strOrigin);
    std::string decrypt(const std::string strOrigin);
    void print();
};

#endif /* TRANSPOSITIONCIPHER_H_ */


.cpp file

#include "transpositioncipher.h"

transpositioncipher::transpositioncipher(std::string KeySecret) {

    mKeySecret = removeDuplicates(KeySecret);
    createOrder();
    for (int i = 97; i &lt; 123; ++i) {
        char c = (char) i;
        std::size_t found = mKeySecret.find(c);
        if (found != std::string::npos) {
            mapOriginAlphabet[c] = false;
        } else {
            mapOriginAlphabet[c] = true;
        }
    }
     createSubstitution();
}

transpositioncipher::~transpositioncipher() {
}

std::string transpositioncipher::removeDuplicates(const std::string strOrigin) {

    std::set&lt;char&gt; moved;
    std::string result;
    result.reserve(strOrigin.size());

    //Only copy one character of each type to the new string
    std::copy_if(std::begin(strOrigin), std::end(strOrigin),
        std::back_inserter(result),
        [&amp;moved](const auto&amp; value) {return moved.insert(value).second;});

     return result;
}

void transpositioncipher::createOrder() {

    std::set&lt;char&gt; chars { mKeySecret.begin(), mKeySecret.end() };

    std::string strTemp = std::string(chars.begin(), chars.end());

    for (auto&amp; x : strTemp) {
        std::size_t found = mKeySecret.find(x);

        vecOrder.push_back(found);
    }

}


void transpositioncipher::createSubstitution() {

    int iCols = mKeySecret.length();
    int iRows = 26 / iCols + 1;

    std::vector&lt;std::vector&lt;char&gt;&gt; vecTemp;

    std::vector&lt;char&gt; vecFirstRow;
    for (auto&amp; x : mKeySecret) {
        vecFirstRow.push_back(x);
    }
    vecTemp.push_back(vecFirstRow);

    int index = 97;
    int iNumRow = 0;

    while (iNumRow &lt; iRows) {
        std::vector&lt;char&gt; row;
        int iNumCol = 0;
        while (iNumCol &lt; iCols) {
            if (mapOriginAlphabet[(char) index]) {
                row.push_back((char) index);
                iNumCol++;
            }
            index++;
            if (index &gt; 123) {
                break;
            }
        }
        vecTemp.push_back(row);
        iNumRow++;
    }

    index = 97;
    for (auto&amp; indexCol: vecOrder) {
        for (auto&amp; vec: vecTemp) {
            if (indexCol &gt;= vec.size())
                continue;
            else {
                mapAlphabet[(char)index] = vec.at(indexCol);
                index ++;
            }
        }
    }
}

std::string transpositioncipher::encrypt(const std::string strOrigin) {
    std::string ret;
    for (auto&amp; x: strOrigin) {
        ret += mapAlphabet[x];
    }

    return ret;
}
I see a number of things that may help you improve your code.

Fix the formatting

The code has an inconsistent format, with varying indentation and brace placement.  To make your code easier to read, you should choose a style and apply it consistently.

Avoid confusing branching constructs

The code currently contains this:

for (auto&amp; vec: vecTemp) {
    if (indexCol &gt;= vec.size())
        continue;
    else {
        mapAlphabet[(char)index] = vec.at(indexCol);
        index++;
    }
}


(with the formatting fixed).  There are several problems with this.  The first is that the control flow is confusing.  It can be made much simpler:

for (auto&amp; vec: vecTemp) {
    if (indexCol &lt; vec.size()) {
        mapAlphabet[(char)index] = vec.at(indexCol);
        index ++;
    }
}


Use appropriate types for variables

The next problem with that code is that it uses an old C-style cast to turn index into a char.  In fact, it's cast to a char just about every time it's used.  Why not just declare it as a char in the first place and remove the casts?

Eliminate "magic numbers"

Instead of using numbers such as 97 and 123 as with this code:

for (int i = 97; i &lt; 123; ++i) {
    char c = (char) i;
    std::size_t found = mKeySecret.find(c);


it would make the code a lot easier to understand if we used the appropriate type of variable and used character constants like this:

for (char c = 'a'; c &lt;= 'z'; ++c) {
    std::size_t found = mKeySecret.find(c);


Use boolean results directly

Rather than this:

std::size_t found = mKeySecret.find(c);
if (found != std::string::npos) {
    mapOriginAlphabet[c] = false;
} else {
    mapOriginAlphabet[c] = true;
}


The code would be much shorter and clearer written like this:

bool found = mKeySecret.find(c) != std::string::npos;
mapOriginAlphabet[c] = !found;


Or possibly this:

mapOriginAlphabet[c] = mKeySecret.find(c) == std::string::npos;


Understand standard template library containers

The code for encrypt contains these lines:

for (auto&amp; x: strOrigin) {
    ret += mapAlphabet[x];
}


This seems to do the right thing, but in fact, it inserts non alphabetic characters into the mapAlphabet which is not the desired behavior here.  Instead, you should use mapAlphabet.at(x) like this:

for (const auto&amp; x: strOrigin) {
    try {
        ret += mapAlphabet.at(x);
    } catch (std::exception &amp;e) {
        // ignore out of range inputs
    }
}


Use const where practical

Performing an encryption or decryption with a given keyword should not alter the underlying transpositioncipher object, so those should be made const.

Prefer const &amp; as function arguments

Several of the functions take a std::string as an argument, but should probably be const std::string &amp; instead to avoid the pointless duplication of a string.  The constructor and removeDuplicates are two such places, as are encrypt and decrypt.

Re-examine your algorithm

What you have implemented is not a transposition cipher, but a substitution cipher in which the ciphertext alphabet is derived by a transposition.  Encryption and decryption could be done with an inverse pair of std::unordered_map objects which may give slightly better performance.

Don't keep variables that are not needed

The vecOrder and mapOriginAlphabet variables are only need during object construction, so they should not be permanent data members of the object.  They could be created and destroyed as the map is being constructed.  Better yet, don't use them at all, as shown in the next suggestion.

Rethink your algorithm

Ultimately, the goal of the constructor is to create a substitution map based on the keyword.  The procedure is to eliminate duplicate letters in the keyword and then to create a table that includes all of the letters, rearranged according to the keyword.  For example, the keyword "bongo" would be reduced to "bngo" and then your vecTemp would look like this:

'b', 'o', 'n', 'g', 
'a', 'c', 'd', 'e', 
'f', 'h', 'i', 'j', 
'k', 'l', 'm', 'p', 
'q', 'r', 's', 't', 
'u', 'v', 'w', 'x', 
'y', 'z', 


We then read down the columns according to the alphabetic order of the letter at the top of each column and create the substitution map:

a = b
b = a
c = f
d = k
e = q
f = u
g = y
h = g
i = e
j = j
k = p
l = t
m = x
n = n
o = d
p = i
q = m
r = s
s = w
t = o
u = c
v = h
w = l
x = r
y = v
z = z


I think that if you ponder it a while longer, there is perhaps a more direct way to derive the map. Instead, you could start with something like this:

std::string &amp;delete_duplicates(std::string &amp;str, size_t len) {
    for (size_t i=0; i &lt; len; ++i) {
        str.erase(std::remove(str.begin()+i+1, str.end(), str[i]),
            str.end());
    }
    return str;
}


Then in the constructor, you could have this:

delete_duplicates(keyword, keyword.size());
std::string alphabet{keyword + "abcdefghijklmnopqrstuvwxyz"};
delete_duplicates(alphabet, keyword.size());


That gives the linear string that represents the two dimensional array shown above. Remap them into vector of strings and sort:          

std::vector&lt;std::string&gt; columns(keyword.size());
for (size_t i=0, col=0; i &lt; alphabet.size(); ++i) {
    columns[col++] += alphabet[i];
    if (col &gt;= columns.size()) {
        col = 0;
    }
}
std::sort(columns.begin(), columns.end());
// now replace the alphabet
alphabet.clear();
for (auto &amp;word : columns) {
    alphabet += word;
}


Now you can construct an encryption map (and a decryption map, if you like) by simply linearly assigning each letter of the "new" alphabet.