Vigenere encryption

Can someone please critique my C++ sample here?

#include    //console io
#include      //string handling
#include   //transform

using namespace std;

struct to_upper {
  int operator() (int ch)
  {
    return toupper(ch);
  }
};

void encrypt_vigenere(const string& plaintext, const string& key, string& ciphertext){
   int i, j, ch;
   for(i = 0, j = 0; i = key.length()) 
         j = 0;
      if(plaintext[i] >= 'A' && plaintext[i] = key.length()) 
         j = 0;

      if(ciphertext[i] >= 'A' && ciphertext[i] <= 'Z')
         ch = ((ciphertext[i] - 'A') + 26 - (key[j] - 'A')) % 26;
      else
         ch = ciphertext[i] - 'A';  //only decrypt A-Z

      plaintext.append(string(1, (char)(ch + 'A')));
   }
}

int main(int argc, char* argv[])
{
   cout << "Enter plaintext to encrypt:\n";
   char plaintext[256] = {0};
   char key[256] = {0};
   cin.getline (plaintext,256);
   cout << "Text to encrypt: " << plaintext << endl;

   cin.clear();  //clears any cin errors 

   cout << "Enter key (can be any string):";

   cin.getline (key, 256);

   //uppercase KEY
   transform(key, key+strlen(key), key, to_upper());

   cout << "key chosen: " << key << endl;

   string cipher;
   //uppercase plaintext
   transform(plaintext, plaintext+strlen(plaintext), plaintext, to_upper());
   encrypt_vigenere(plaintext, key, cipher);

   string decrypted;
   decrypt_vigenere(cipher, key, decrypted);

   cout << "Vigenere encrypted text: " << cipher << " decrypted: " << decrypted << endl;

return 0;
}

General Coding Style

Please don't do this:

using namespace std;

I know every C++ books starts with this line. Fine it works for ten line programs they show in books. But when you start writing real programs this becomes a pain because of the clashes it can introduces as a result real programs will not have this in them. So get used to not using it.

There are a couple of alternatives:

• Prefix everything with std::

• Use the using declaration to bring specific types/objects from a namespace

• Be selective and bind them as tight as you can.

There is already a toupper() defined by the standard (and you are using it).

struct to_upper {
  int operator() (int ch)
  {
    return toupper(ch);
  }
};

Just use that in your transform (note you will need to specify ::toupper).

std::transform(begin, end, dst, ::toupper);

One variable per line:

int i, j, ch;

And make them readable (you don't need to obtuse and make them 100 characters) but one character variables are a pain when you are trying to spot all their uses in you code. Also note You can declare loop variables as part of the for(). Declare your variables as close to the use point as you can.

Be consistent in your usage:

for(i = 0, j = 0; i < plaintext.length(); ++i, j++) {

Why is it ++i yet you use j++. It makes me look at the code to see if there is something special I should notice (there does not seem to be). So you are wasting my time makeing me thing why is he doing that.

Personally (and this is just a thing I would do you can take it or leave it). I would replace this:

if(j >= key.length()) 
     j = 0;

With (obviously removing it from the for() aswell):

// Increment and wrap `j`
  j = (j + 1) %  key.length();

Pretty sure you de-crypt is not working.

if(plaintext[i] >= 'A' && plaintext[i] <= 'Z')
     ch = ((plaintext[i] - 'A') + (key[j] - 'A')) % 26;
  else
     ch = plaintext[i] - 'A';  //only encrypt A-Z
  ciphertext.append(string(1, (char)(ch + 'A')));

If you are going to read user input then use a std::string. It works in exactly the same way and makes things easier (you don't need to detect and fix very long strings).

char plaintext[256] = {0};
cin.getline (plaintext,256);

// Easier as

std::string plaintext;
std::getline(std::cin, plaintext);

Again declare your variables as close to the point you are going to use them. Don't get trapped in the C style of putting the variables at the top of the function.

OK. So you clear the error state.

cin.clear();  //clears any cin errors

But you don't really do anything about it.

This gives you a false sense of security. Best avoided. If you are going to handle errors do so properly or not at all.

Algorithm usage

You obviously have seen how the standard libs use algorithms.

transform(key, key+strlen(key), key, to_upper());

There are a couple of other places you can use the standard algorithms to make your code clearer. The encryption and de-cryption loops are exactly the place this kind of things work well in:

for(i = 0, j = 0; i = key.length()) 
     j = 0;
  if(plaintext[i] >= 'A' && plaintext[i] <= 'Z')
     ch = ((plaintext[i] - 'A') + (key[j] - 'A')) % 26;
  else
     ch = plaintext[i] - 'A';  //only encrypt A-Z
  ciphertext.append(string(1, (char)(ch + 'A')));
}

I would re-write as:

ciphertext.resize(plaintext.size());
std::transform(plaintext.begin(), plaintext.end(), ciphertext.begin(), MyCrypt(key));

Then just write your crypt function as a nice functor:

struct MyCrypt
{
            std::string key;
    mutable std::size_t keyIndex;

    MyCrypt(std::string const& key): key(key), keyIndex(0) {}
    char operator()(char const& plain) const
    {
        char keyChar = key[keyIndex] - 'A';
        keyIndex     = (keyIndex + 1) % key.size();

        return (plain >= 'A' && plain <= 'Z')
                   ? (plain - 'A' + keyChar) % 26;
                   : plain - 'A';
    }
};

Your function

I would take this a step further and make your function work with any input type. So you are basically applying the standard convention of using iterators to specify your inputs/outputs and your encryption falls into that category.

Your algorithm should not care where the plain text is coming from nor where it is going too.

So I would change your function interface too:

template
void encrypt_vigenere(string const& key, II begin, II end, IO dst)
{
    std::transform(begin, end, dst, MyCrypt(key));
}

Now when you call it you don't even need to copy things into strings you can just encrypt the input into the output:

encrypt_vigenere("My long key that nobody well guess",
                 std::istreambuf_iterator(std::cin),
                 std::istreambuf_iterator(),
                 std::ostream_iterator(std::cout)
                );

Update: Correct way to write functor:

```
// Alternative Version 1:
struct MyCrypt
{
std::string key;

StackExchange Code Review Q#10894, answer score: 21