current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite
1

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.

Below is my implementation of ROT47 algorithm:

#include <cstring>
#include <cstdio>
#include <iostream>

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.

share|improve this question
    
Sorry if I'm missing something fundamental about ROT-47, but can't the new character just be determined with math rather than searching through a string? What you have is great for a general-purpose substitution cipher since you can change each of your strings to whatever you want, but ROT-47 can be much simpler, right? You just need to add 47 to the current character and then handle the wrap-around. –  JPhi1618 22 hours ago
    
@JPhi1618 - that works if your system uses ASCII (which it probably does). It doesn't work with encodings such as EBCDIC. –  Pete Becker 17 hours ago

3 Answers 3

up vote 12 down vote accepted 
#include <cstring>
#include <cstdio>

You're not using either of those, so remove them, and include <string> 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.

share|improve this answer
    
Thank you for all those tips ! :) –  Aleph0 yesterday
1  
+1 for the last paragraph on variable names. FAR too few people actually worry enough about these. –  Nate Kerkhofs yesterday
up vote 2 down vote

As I see this, your code would behave strangely when fed non-ASCII characters. It iterates through all bytes in the string, and will thus do strange things with multibyte-characters like in the UTF-8 encoding.

You could try it yourself by converting "Zurück" with the code.

This isn't that easy of a fix unfortunatly. You'd either need to explicitly support UTF-8, or do hacky things like aborting/erroring when a character greater than 0x7F is inputted (because you can't know the length of the following bytes).

share|improve this answer
    
This is a good catch, but given that ROT47 is based on ASCII, I think the best fix is documenting that this function is intended for use with ASCII strings and behavior with other encodings is undefined. Trying to detect different encodings based on byte patterns is starting down a very, very deep rabbit hole. –  GrandOpener 22 hours ago
    
@GrandOpener - but if you require ASCII for input the implementation can be much simpler: just add 47, and adjust if the result is too large. –  Pete Becker 17 hours ago
    
@PeteBecker - Yes, I completely agree. If I were writing an answer, I would include that suggestion. –  GrandOpener 16 hours ago
    
I know it won't work for non-ASCII characters but this is related to the algorithm itself. Anyway, I didn't plan to use it on non-ASCII characters in my case, thus it's not really a problem for me. Thank you for your answer. :) –  Aleph0 5 hours ago
up vote 2 down vote

EBCDIC is a myth made up to scare children during holidays, so here is the ASCII-dependent version, as suggested in a comment by JPhi1618.

The main imrovement is to efficiency, by avoiding std::string::find, which is probably implemented as a linear search.

Unicode is not a myth, unfortunately, so don't ever try encrypting the string "Zurück".

    #include <string>
    #include <iostream>

    std::string rot47(const std::string& s) {
        std::string ret;
        for(const char plain : s) {
            const char crypt = '!' + (plain - '!' + 47) % 94;
            ret += crypt;
        }
        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;
    }
share|improve this answer
    
That is a nice way to do it, thank you for your answer. ;) –  Aleph0 5 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.