Encode a string using Caesar's Cipher

Question

My program is complete and running, but I would like second opinions on how I can make it more efficient. The program prompts the user for the message and the amount s/he would like to shift the letters. For the implementation part, is there another "faster" way to do it instead of having the if else if statements to run through and test my characters?

#include "stdafx.h"
#include <iostream> 
#include <string> 

using namespace std; 

string encodeCaesarCipher(string str, int shift); 

int main()
{
    string str, encoded_string;  
    int shift; 

    cout << "This program encodes a message in Caesar Cipher. \n\n"; 

    cout << "Please enter a message: "; 
    getline(cin, str);  

    cout << "\nHow many times do you want to shift the letter(s) in the message? ";
    cin >> shift; 

    while (shift < 1)
    {
        cout << "You have entered an invalid number. \n"; 
        cout << "Please try again. \n"; 
        cout << "Enter a proper number for the amount of letter shifting: "; 
        cin >> shift; 
    }

    encoded_string = encodeCaesarCipher(str, shift); 

    cout << "\nEncoded message: " << encoded_string << "\n\n"; 

    system("PAUSE"); 

    return 0;
}

string encodeCaesarCipher(string str, int shift)
{
    string temp = str; 
    int length; 

    length = (int)temp.length(); 

    for (int i = 0; i < length; i++)
    {
        if (isalpha(temp[i]))
        {
            for (int j = 0; j < shift; j++)
            {
                if (temp[i] == 'z')
                {
                    temp[i] = 'a'; 
                }
                else if (temp[i] == 'Z')
                {
                    temp[i] = 'A'; 
                }
                else 
                {
                    temp[i]++; 
                }
            }
        }
    }

    return temp; 
}

Answer 1

Optimization difficulty is about quadratic on the length of a function. I suggest a separate shift function, like

 char shift(char ch, int shift)
 {
            if (ch == 'z')
            {
                 return 'a'; 
            }
            else if (ch== 'Z')
            {
                return 'A'; 
            }
            else 
            {
                return ch + 1;  
            }
   }

And you can call it in the loop.

Anyhow this programme does not need speed up, runtime is dominated by input and output taking.

Avoid namespace std;, return 0, and system("pause"). The first causes name clashes, the second is not needed, the third is a portability issue and a security breach.

As a minor note, declare and assign length on the same line:

  int length = (int) str.length()

Following @Ryan 's suggestion, -25 can be used to merge 'z' and 'Z'. It then becomes so short to beg for a ternary:

 char shift(char ch, int shift)
 {
     return ch == 'z' || ch == 'Z' ? ch - 25 : ch + 1;
 }

Answer 2

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It is particularly bad to put it into a header file, so please don't do that.

Don't use system("PAUSE")

There are two reasons not to use system("cls") or system("PAUSE"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named PAUSE or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example:

void pause() {
    getchar();
}

General portability

This code could be made portable if, in addition to the changes in the previous point, you omit the Windows-only include files #include "stdafx.h".

Use const where practical

The passed str is not modified by encodeCaesarCipher() and so it should be declared as const std::string &. Right now, it's passed by value, so the code is making a useless extra copy.

Use constant string concatenation

Within main the code has several places where successive lines of code do nothing except emit a constant string to std::cout using operator <<. Instead of making separate calls to operator <<, you could call it just once:

std::cout << "You have entered an invalid number. \n" 
    "Please try again. \n" 
    "Enter a proper number for the amount of letter shifting: "; 

This only calls << once instead of three times. The compiler automatically concatenates the string literals together.

Use "range-for" to simplify your code

Instead of using an index variable, your for loop could use "range-for":

for (auto &ch : temp)

Consider an alternative approach

Rather than doing all of that calculation for each letter, you could use a simple lookup mechanism instead. Keep two contiguous alphabets in memory and use the plaintext char and rotate value to calculate an offset.

int encode(int ch, int rotate)
{
    const char alphabet[] = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz";
    if (std::islower(ch)) {
        return alphabet[ch-'a'+rotate];
    } else {
        return std::toupper(alphabet[std::tolower(ch)-'a'+rotate]);
    }
}

std::string encodeCaesarCipher(const std::string &str, int shift)
{
    std::string temp{str}; 
    for (auto &ch : temp) {
        if (std::isalpha(ch)) {
            ch = encode(ch, shift);
        }
    }
    return temp; 
}

With some minor rearrangement and a lambda, one could even turn encodeCaesarCipher into a one-line function using std::transform, but I'll leave that for you.

Answer 3

Not an experienced C++ programmer, so this will be more nitpicks.

Again, it's not a very good idea to do something like this:

system("PAUSE");

This won't be available on all systems, and it's not readable to someone who is reading your code. It's also slow and insecure as well.

I'd suggest doing something like this, rather than the above:

std::cin.get();

This line needs to go:

 using namespace std;

It's a horrible habit to get into, and doing stuff like this can introduce all sorts of issues if there are name conflicts. Many of the negative side effects can be seen here.

Finally, you can get rid of this as well:

return 0;

main is a special function, so the above will automatically be inserted by the compiler if it isn't found.