ROT47 Implementation

ROT13 ("rotate by 13 places", sometimes hyphenated ROT-13) is a simple letter substitution cipher that replaces a letter with the letter 13 letters after it in the alphabet. ROT13 is a special case of the Caesar cipher, developed in ancient Rome.


ROT47 is a derivative of ROT13 which, in addition to scrambling the basic letters, also treats numbers and common symbols. Instead of using the sequence A–Z as the alphabet, ROT47 uses a larger set of characters from the common character encoding known as ASCII. Specifically, the 7-bit printable characters, excluding space, from decimal 33 '!' through 126 '~', 94 in total, taken in the order of the numerical values of their ASCII codes, are rotated by 47 positions, without special consideration of case. For example, the character A is mapped to p, while a is mapped to 2.

Below is my implementation of ROT47 algorithm:

#include 
#include 
#include 

std::string rot47(std::string s)
{
    std::string s1 = "!\"#$%&\'()*+,-./0123456789:;?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~";
    std::string s2 = "PQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~!\"#$%&\'()*+,-./0123456789:;?@ABCDEFGHIJKLMNO";

    std::string ret = "";
    for (unsigned int i = 0; i < s.size(); i++)
    {
        std::size_t pos = s1.find(s[i]);
        ret += s2[pos];
    }

    return ret;
}

int main()
{
    std::string str = "HelloWorld!";

    std::string retFct = rot47(str);
    std::cout << retFct << std::endl;

    std::string retFct2 = rot47(retFct);
    std::cout << retFct2 << std::endl;
}

It works as expected but my question is : Is there something I can improve in my implementation? I don't know if there is a more direct way to get the result.

#include 
#include 

You're not using either of those, so remove them, and include ` instead to get std::string.

std::string rot47(std::string s) { /* ... */ }

Why copy the caller's string? You're only reading from it, so take it by const&.

std::string s1 = "!\"#$%&\...";
std::string s2 = "PQRSTUVW...";

These two are constant, don't allocate them for every call, make them static (and const):

static std::string const s1 = "...";

std::string ret = "";

That initialization is not necessary, a default-constructed string is empty. You might want to reserve the appropriate size since you know it though, for optimal performance.

for (unsigned int i = 0; i < s.size(); i++) { /* ... */ }

You can use range-based for loop with strings:

for (auto c: s) { /* ... */ }

And you should probably check that s1.find actually found something, otherwise the line after that is undefined behavior.

And s, s1 and s2 are rather poor choices for those variable names. Something like "source", "original", "plaintext" for the input would be better. map_from / map_to` could work for the other two.

StackExchange Code Review Q#106706, answer score: 15