Return to Answer

• 1

• 67.2k
• 4
• 120
• 284

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.