1
\$\begingroup\$

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?

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

2 Answers 2

1
\$\begingroup\$

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
\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$
    – esote
    Jan 2, 2017 at 17:25
1
\$\begingroup\$

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! :)

\$\endgroup\$
2
  • 3
    \$\begingroup\$ 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. \$\endgroup\$ Jan 2, 2017 at 11:17
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Jan 2, 2017 at 14:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.