HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

ROT13 implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationrot13stackoverflow

Problem

I'm doing some coding challenges to get better practiced at my coding. I'm trying to do one that requires us to do a ROT13. I've got the implementation correct, and I just want to know a couple of things.

The space character is showing up as a crazy character in my terminal screen, which I'm assuming is because of character encoding, although I could be wrong. Also, how does my code look?

#include 
#include 
using namespace std;

const int LOWER_A = 97;
const int LOWER_M = 109;
const int LOWER_N = 110;
const int LOWER_Z = 122;

const int UPPER_A = 65;
const int UPPER_M = 77;
const int UPPER_N = 78;
const int UPPER_Z = 90;

string rot(string input) {
    int inputSize = input.size();
    int index = 0;

    while(index != inputSize) {
        if(input[index] >= LOWER_A && input[index] = LOWER_N && input[index] = UPPER_A && input[index] <= UPPER_M)
            input[index] = input[index] + 13;
        else if(input[index] <= UPPER_N && input[index] <= UPPER_Z)
            input[index] = input[index] - 13;

        index++;
    }
    return input;
}

int main() {
    string plaintext;
    string cypher;

    cout << "input: ";
    getline(cin,plaintext);
    cypher = rot(plaintext);
    cout << cypher << endl;

    return 0;
}

Solution

A few fairly minor things:

Constants for the characters

Instead of using the constants, you can just use a character inline:

if(input[index] >= 'a' && input[index] <= 'm')


It's worth noting though that a literal character will be a char rather than an int like your constants are (won't affect anything in this program though).

using namespace std;

It's fine in short programs like this, but as you hinted that you're fairly new to C++, I wanted to mention that this can be a bad habit to get into. In particular, in large applications with many different segments of code, this can cause naming clashes.

This question on SO addresses the problems it can cause.

Naming

I would probably call rot, rot13 since there are other kinds of rotations. This is mainly me just being overly picky though :).

Strange Character For Space


The space character is showing up as a crazy character in my terminal screen, I'm assuming it's because of character encoding, although I could be wrong.

rot13 traditionally only operates on alpha characters (a-z and A-Z), so your space characters should not be changing.

I believe your program has a bug in it:

else if(input[index] <= UPPER_N && input[index] <= UPPER_Z)


If this is not a bug, you should know that this will be always be true if input[index] <= UPPER_N, so really there's no point in the second conditional.

Anyway, I think what's happening is that the space character (and anything under 'N') is falling into this block when it shouldn't be.

Space is ASCII 32, so it's getting changed to 19, which is not a printable character.

while vs for

Your loop seems to fit the construct of a for loop very well, so I might consider using that:

for (int inputSize = input.size(), index = 0; index != inputSize; ++index) {

}


This comes down to personal preference though.

You should usually try to match the return type of a method

std::string::size() doesn't return an int; it returns a std::string::size_type which is always (in every implementation I've ever seen anyway), a size_t (which is in turn an unsigned 32 or 64 bit integer depending on the platform and compiler).

Anyway, when working with standard containers (or really APIs in general), unless you have a reason to use a different type, I would try to stick with the return type specified. For example:

for (std::string::size_type len = input.size(), idx = 0; idx != len; ++idx) {
    //input[idx] = ...;
}


A potential implementation

I suspect that the if-else tree can be simplified a bit using isalpha and family, but this is what I might implement your algorithm like;

std::string rot13(std::string input) {

    for (std::string::size_type len = input.length(), idx = 0; idx != len; ++idx) {
        if (input[idx] >= 'a' && input[idx] = 'n' && input[idx] = 'A' && input[idx] = 'N' && input[idx] <= 'Z') {
            input[index] = input[index] - 13;
        }
    }

    return input;

}


To get a little carried away for a moment, you could also implement it using iterators. Using iterators would allow you to apply it to any container that implements iterators (all of the standard containers and plain pointers).

template 
void rot13(Iter begin, const Iter& end) {

    while (begin != end) {

        //Doesn't need to be here, but I'm lazy and don't like
        //typing *begin over and over again.
        char& c = *begin;

        if (c >= 'a' && c = 'n' && c = 'A' && c = 'N' && c <= 'Z') {
            c -= 13;
        }

        ++begin;

    }

}


Note that this modifies a container in place rather than creating a copy:

char str[] = "Hello World";
rot13(str, str + strlen(str));
std::cout << str << std::endl;
rot13(str, str + strlen(str));
std::cout << str << std::endl;


Would output:

Uryyb Jbeyq
Hello World


You could of course modify it to operate on a copy instead though.

Code Snippets

if(input[index] >= 'a' && input[index] <= 'm')
else if(input[index] <= UPPER_N && input[index] <= UPPER_Z)
for (int inputSize = input.size(), index = 0; index != inputSize; ++index) {

}
for (std::string::size_type len = input.size(), idx = 0; idx != len; ++idx) {
    //input[idx] = ...;
}
std::string rot13(std::string input) {

    for (std::string::size_type len = input.length(), idx = 0; idx != len; ++idx) {
        if (input[idx] >= 'a' && input[idx] <= 'm') {
            input[idx] = input[index] + 13;
        } else if (input[idx] >= 'n' && input[idx] <= 'z') {
            input[idx] = input[idx] - 13;
        } else if(input[idx] >= 'A' && input[idx] <= 'M') {
            input[index] = input[index] + 13;
        } else if(input[idx] >= 'N' && input[idx] <= 'Z') {
            input[index] = input[index] - 13;
        }
    }

    return input;

}

Context

StackExchange Code Review Q#14569, answer score: 6

Revisions (0)

No revisions yet.