Simple Caesar Cipher C++

Question

I was surprised to learn that there are not many C++ Caesar ciphers on Code Review (3 others as of this question). And none that work in this fashion.

Here's the code:

// Caesar cipher

#include <iostream>

const char chars[] = {"abcdefghijklmnopqrstuvwxyz"};

// Find position in array
int findPosition(char x){
    int i;

    for(i = 0; i <= 25; i++){
        if(x == chars[i]){
            break;
        }
    }

    return i;

}


// Change char to next in array, loop back if at end
char nextChar(char x){
    if(findPosition(x) == 25){
        return chars[0];
    }

    return chars[findPosition(x) + 1];
}

// Rotate char desired amount
char rotChar(char x, int rotNum){
    for(int i = 1; i <= rotNum; i++){
        x = nextChar(x);
    }

    return x;
}

int main(){
    std::string str;
    std::getline(std::cin, str);

    std::cout << '\n';

    for(int i = 0; i <= 25; i++){
        std::cout << "ROT" << i << " - ";

        for(int k = 0; str[k] != '\0'; k++){
            std::cout << rotChar(str[k], i);
        }

        std::cout << '\n';
    }

    return 0;
}

This is a very simple Caesar cipher. It currently ignores anything uppercase or non-alphabetic.

Here's an example output:

ROT0 - helloworld
ROT1 - ifmmpxpsme
ROT2 - jgnnqyqtnf
ROT3 - khoorzruog
ROT4 - lippsasvph
ROT5 - mjqqtbtwqi
ROT6 - nkrrucuxrj
ROT7 - olssvdvysk
ROT8 - pmttwewztl
ROT9 - qnuuxfxaum
ROT10 - rovvygybvn
ROT11 - spwwzhzcwo
ROT12 - tqxxaiadxp
ROT13 - uryybjbeyq
ROT14 - vszzckcfzr
ROT15 - wtaadldgas
ROT16 - xubbemehbt
ROT17 - yvccfnficu
ROT18 - zwddgogjdv
ROT19 - axeehphkew
ROT20 - byffiqilfx
ROT21 - czggjrjmgy
ROT22 - dahhksknhz
ROT23 - ebiiltloia
ROT24 - fcjjmumpjb
ROT25 - gdkknvnqkc

How can this code be improved?

Answer 1

Function by function review

findPosition()

This function is inefficient because it searches an alphabetized string trying to find the index of a lowercase letter. It would be faster to not even use the string and just compute the position directly like this:

if (x >= 'a' && x <= 'z')
    return x - 'a';

The other problem is what to return if the input character is not found. Currently you return 26 and this causes the caller to print a null character (because chars[26] is the end of the string). I think it would be preferable to return -1 and have the caller detect and deal with that. So in total:

int findPosition(char x)
{
    if (x >= 'a' && x <= 'z')
        return x - 'a';
    return -1;
}

nextChar()

This function works fine, but it gets called n times, where n is the number of times you are rotating. You should be able to delete this function if you modified rotChar() appropriately.

rotChar()

Instead of calling nextChar() rotNum times, you should be able to use the modulo operator to compute the next character in one step:

// Rotate char desired amount
char rotChar(char x, int rotNum)
{
    int pos = findPosition(x);

    if (pos == -1)
        return x;
    return 'a' + ((pos + rotNum) % 26);
}

Notice that this also returns the character unmodified if it wasn't a lowercase character. At this point, you might want to just eliminate findPosition() and just do everything in rotChar() like this:

// Rotate char desired amount
char rotChar(char x, int rotNum)
{
    if (x >= 'a' && x <= 'z')
        return 'a' + (((x - 'a') + rotNum) % 26);
    return x;
}

Then you could easily extend this to handle uppercase letters as well:

// Rotate char desired amount
char rotChar(char x, int rotNum)
{
    if (x >= 'a' && x <= 'z')
        return 'a' + (((x - 'a') + rotNum) % 26);
    if (x >= 'A' && x <= 'Z')
        return 'A' + (((x - 'A') + rotNum) % 26);
    return x;
}

Answer 2

Magic numbers are bad. What is this 25 everywhere? Were does it stem from? What does it mean? Of course, that's the number of characters in your alphabet, but the stray number 25 doesn't show that information.

So we should change that:

const int ALPHABET_SIZE = 26; // see better version below

Also, make sure that your alphabet has a good name. It's a global constant variable:

const char ALPHABET[] = {"abcdefghijklmnopqrstuvwxyz"};

But we should make sure that ALPHABET_SIZE is always in sync with ALPHABET, so we should use

const int ALPHABET_SIZE = sizeof ALPHABET / sizeof char;

Next, your findPosition returns invalid values if the character isn't in the alphabet. That can then lead to problems if you use ALPHABET[26]. But that's hard too catch since i lives too long:

// Find position in array
int findPosition(char x){
    int i;

    for(i = 0; i <= 25; i++){
        if(x == ALPHABET[i]){
            break;
        }
    }
    return i; // <---  i might be 26 here
}

Try to minimize the scope of a variable. Note that you should come up with some kind of "error" return value if the character is not in your alphabet. -1 is often used in the C world:

const int INVALID = -1;

// Find position in array
int findPosition(char x){
    for(int i = 0; i < ALPHABET_SIZE; i++){
        if(x == ALPHABET[i]){
            return i;
        }
    }

    return INVALID; // cannot return an invalid 'i' here, since 'i' is out of scope
}

Now you have to check whether the position is actually valid:

char nextChar(char x){
    const int position = findPosition(x);
    if(position == INVALID) {
        return x;
    } else if (position == ALPHABET_SIZE){
        return chars[0];
    }

    return ALPHABET[position + 1];
}

Note that your code called findPosition twice. Your rotChar is fine, apart from the fact that most C/C++ programmer use i = 0 and i < upperLimit instead of i = 1; i <= upperLimit. But that's personal preference.

What's not fine is the overhead. You should merge both nextChar and rotChar into a single function:

char rotChar(char x, int rotNum) {
    const int position = findPosition(x);

    if(position == INVALID) {
        return x;
    }

    return ALPHABET[(position + rotNum) % ALPHABET_SIZE];
}

We end up with the following code:

const char ALPHABET[] = {"abcdefghijklmnopqrstuvwxyz"};
const int ALPHABET_SIZE = sizeof ALPHABET / sizeof char;

const int INVALID = -1;

int findPosition(char x){
    for(int i = 0; i < ALPHABET_SIZE; i++){
        if(x == ALPHABET[i]){
            return i;
        }
    }

    return INVALID;
}

char rotChar(char x, int rotNum) {
    const int position = findPosition(x);

    if(position == INVALID) {
        return x;
    }

    return ALPHABET[(position + rotNum) % ALPHABET_SIZE];
}

Your main can stay the same, but I wouldn't put one character after another. Instead, write a function

std::string rotString(std::string);

that rotates a string at once, so that you can transform a whole line instead of a single character.