Getting Input Key for Atbash Cipher

Question

An Atbash cipher is a simple substitution cipher wherein a key is used to determine what letters to swap, for example:

Original alphabet: abcdefghijklmnopqrstuvwxyz
Substitute (key): zyxwvutsrqponmlkjihgfedcba
Original message: hello world
Altered message: svool dliow

In my program, I get the key by printing the original letter, and then getting the letter that is to substitute it, and adding it to a key string, for example:

a = <user input>
b = <user input>
c = <user input>
...

Here is the simple code that I use for it:

std::cout << "Key:\n";
std::string key;
for(int i = 0; i <= 25; i++){
 std::cout << static_cast<char>('a' + i) << " = ";
 char inputChar;
 std::cin >> inputChar;
 key += inputChar;
 std::cin.ignore();
 std::cin.clear();
 std::cout << std::flush;
}

How can I improve this code?

• πάντα ῥεῖ: I think the std::cin.ignore(); and std::cin.clear(); are superfluous.
• Incomputable: @πάνταῥεῖ, it is implementation defined if they are superfluous, but in practice (windows-linux), yeah, pretty much.
• πάντα ῥεῖ: @Incomputable Not really implementation defined. Reading a sequence of chars,- whitespace delimited or not -, should work fine in any implementation.
• Incomputable: @πάνταῥεῖ, I meant the ignore-clear-flush sequence. It is implementation defined for cin and cout that they actually ignore-clear-flush on death
• πάντα ῥεῖ: @Idempotence For better UI experience you should let know the user what is expected and what will be ignored.

Answer 1

How can I improve this code?

Fix a bug ;). At the moment, one can accidentally enter the same character twice, e.g.:

a = a
b = a
c = a
...
z = a

Unless you want to allow that usage, you probably want to check which characters are already used:

for(int i = 0; i <= 25; i++){
 std::cout << static_cast<char>('a' + i) << " = ";
 char inputChar;
 std::cin >> inputChar;
 if(key.find(inputChar) == std::string::npos){
 key += inputChar;
 } else {
 // yell at user!
 }
 std::cin.ignore();
 std::cin.clear();
 std::cout << std::flush;
}

Also, if you know the size of the std::string in advance, you can speed up append operations (such as +=) with reserve:

std::string key;
key.reserve(26);
// ... rest of your code

• esote: Thanks, I forgot to add a test for such an error. You're answer covers it most completely. I also changed key += inputChar to key += tolower(inputChar) because entering capital letters breaks the find(inputChar) part.

Answer 2

For one thing, it is very inadvisable to use the += operator with strings, because it's slow and it requires the string to be relocated in the memory. This isn't much of a problem with higher-level languages like Python, but with C and also Java, it's not a good idea, and it's not good practice. Since you know that there are 26 letters in the alphabet, you can make a length 26 array to store the key instead. Otherwise, the rest of the code looks good! :)

• Cody Gray: You could simply call reserve on the allocated string object to avoid unnecessary allocations. Then, using += would be totally fine and entail no performance penalty.
• Cody Gray: Amortized constant doesn't mean free, @zeta. You have to understand what is actually going on under the hood. It performs an allocation and copy if there is not enough space available in the backing buffer. It grows the buffer by a constant multiplier (1.5x or 2x are common for most implementations), with the idea that you won't need to allocate additional space each time you append, which is where the "amortized" part comes in. But allocations still have to happen, and you won't necessarily benefit from the amortization. Reserving enough space in advance is always the most performant choice.