17
\$\begingroup\$

Can someone please critique my C++ sample here?

#include <iostream>   //console io
#include <string>     //string handling
#include <algorithm>  //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 < plaintext.length(); ++i, j++) {
      if(j >= 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')));
   }
}

void decrypt_vigenere(const string& ciphertext, const string& key, string& plaintext){
   int i, j, ch;
   for(i = 0, j = 0; i < ciphertext.length(); ++i, j++) {
      if(j >= 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;
}
\$\endgroup\$
20
\$\begingroup\$

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 < plaintext.length(); ++i, j++) {
  if(j >= 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<typename II, typename IO>
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<char>(std::cin),
                 std::istreambuf_iterator<char>(),
                 std::ostream_iterator<char>(std::cout)
                );

Update: Correct way to write functor:

// Alternative Version 1:
struct MyCrypt
{
    std::string  key;
    std::size_t& keyIndex;

    MyCrypt(std::string const& key, std::size_t& keyIndex)
        : key(key)
        , keyIndex(keyIndex)
    {
        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';
    }
};


// Usage:
std::size_t  keyIndex;
std::transform(begin, end, dst, MyCrypt(key, keyIndex));

Or:

// Alternative Version 2
struct MyCrypt
{
    std::string  key;
    std::size_t  keyIndex;

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

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


// Usage:
MyCrypt  crypt(key);
std::transform(begin, end, dst, crypt);

Strictly speaking either of these would be more correct.
But I find both have disadvantages. The first version requires that you keep the index outside the functor (so that functor can retain its const(ness) while mutating the keyIndex). While the second version requires you to remove the const(ness) of the operator() which also means you can not dynamical create and pass it as a parameter (as it would be a temporary and thus us required to be const) to the algorithms. Instead you need to create an object and pass the object.

Thus I prefer using the method I initially showed above (it bends the rules) but in my opinion makes the code easier to read/maintain and understand.

\$\endgroup\$
  • \$\begingroup\$ There is but one thing I could add to this answer, so I'm not answering separately: if you have a capable compiler, a lambda might be even more suitable than a hand-crafted functor (you get the same thing but "in-place"). \$\endgroup\$ – Tamás Szelei Apr 15 '12 at 0:56
  • \$\begingroup\$ A lambda wont be suitable in this case as state needs to be maintained (keyIndex). Hence why a struct functor is used instead of a function. \$\endgroup\$ – cmh Apr 16 '12 at 13:27
  • \$\begingroup\$ You could capture those as variables from the outside scope. \$\endgroup\$ – Tamás Szelei Apr 17 '12 at 7:49
  • \$\begingroup\$ This is incorrect, though it probably works on most implementations. See en.cppreference.com/w/cpp/algorithm/transform: "std::transform does not guarantee in-order application of unary_op or binary_op." \$\endgroup\$ – ruds Apr 12 '14 at 21:09
  • \$\begingroup\$ @ruds: incorrect. std::transform() uses Input Iterator Input iterator only have the capability for forward movements (with operator++) then can not move backwards and they can not randomly move between elements. So effectively this means transform must act upon the elements in order. \$\endgroup\$ – Martin York Apr 12 '14 at 21:56
9
\$\begingroup\$

Return Values

I would modify the encrypt and decrypt functions to return a string value rather than take a string reference.

From a design point of view this bring several advantages:

  1. It never makes more sense to use the reference to provide any input other than the empty string, so I would codify this in the parameter list.
  2. Any compiler worth its salt will apply a return value optimization, so there should be no performance hit.
  3. It de-clutters the call site and improves readability.

String Concatenation

Prefer

ciphertext += (char)(ch + 'A');

over

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

For any reasonable implementation, there should be no performance difference.


Here's the amended code:

string encrypt_vigenere(const string& plaintext, const string& key){
    string ciphertext;
    int i, j, ch;
    for(i = 0, j = 0; i < plaintext.length(); ++i, j++) {
        if(j >= 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 += (char)(ch + 'A');
     }
     return ciphertext;
}

string decrypt_vigenere(const string& ciphertext, const string& key){
    string plaintext;
    int i, j, ch;
    for(i = 0, j = 0; i < ciphertext.length(); ++i, j++) {
        if(j >= 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 += (char)(ch + 'A');
     }
     return plaintext;
}


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;

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

    string decrypted = decrypt_vigenere(cipher, key);

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

    return 0;
}

On a more minor note, I'm not a big fan of this comment:

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

The comments should explain the why, not the what (which is already self evident). I would prefer a comment explaining why you always return the uppercase of the key and cipher.

\$\endgroup\$
  • \$\begingroup\$ In the line transform(key, key+strlen(key), key, to_upper()) to_upper() will cause a syntax error. \$\endgroup\$ – Tamás Szelei Apr 15 '12 at 1:03
  • 1
    \$\begingroup\$ @afishwhoswimsaround No syntax error there, to_upper is a struct in the asker's code (Which happens to be a functor struct), so to_upper() will create a temporary default-constructed to_upper object \$\endgroup\$ – Ben Cottrell Apr 15 '12 at 9:03
  • \$\begingroup\$ You are right, I missed that. \$\endgroup\$ – Tamás Szelei Apr 15 '12 at 10:17
8
\$\begingroup\$

In addition to what cmh and Loki have said, here are a few pet peeves of mine:

  1. Ban C-style casts from your code-base. They are dangerous (they are an everything-goes cast which severely undermines the type system) and too innocuous. Casts should stand out.

    So instead of (char) foo, use static_cast<char>(foo); yes, it’s long and ugly. That’s intentional to make the cast stand out.

  2. Break the logic into smaller non-repeating blocks. With Loki’s amendments this is obsolete but in general look how similar your encrypt and decrypt functions are. You should strive to reduce this redundancy. If I remember my Vigenère correctly these functions are symmetrical and can be implemented by the same logic.

    You are also converting quite a lot between chars and numbers by adding or subtracting 'A'. Encapsulate this into a pair of functions, it makes the code cleaner.

\$\endgroup\$
5
\$\begingroup\$

Other than (perhaps) avoiding classifying other "things" as upper-case letters, I'm not sure why you'd use x >= 'A' && x <= 'Z' instead of isupper(x).

I'm also a bit bothered by having key[index] - 'A' where the key is used -- I'd prefer to transform the entire key to the key[i]-'A' at initialization, and just use those values from then on.

I don't think the encryption really works correctly as given. In particular, I believe it transforms 'A' .. 'Z' into the range 0..26 instead of producing output in the range 'A'..'Z' again.

I think a bit more can also be done to merge the code for encrypting and decrypting together -- basically the only difference between them is adding vs. subtracting at one point. To keep as much code in common as possible, I'd use code something like this:

class BaseCrypt { 
    int clamp(int val, int max) {
        if (val < 0) val += max;
        else if (val >= max) val -= max;
        return val;
    }
protected:
    std::string key;
    size_t index;

    template <class xfm>
    char xform(char input) {
        static unsigned const int letters = 26;
        if (!isupper(input))
            return input;
        int pos = input - 'A';
        int e_pos = xfm()(pos, key[index]);
        e_pos = clamp(e_pos, letters);
        char val = e_pos + 'A';
        index = (index + 1) % key.length();
        return val;
    }
public:
    BaseCrypt(std::string init) : index(0) { 
        std::transform(
            init.begin(), 
            init.end(), 
            std::back_inserter(key),
            [](char ch) { return ::toupper(ch) - 'A'; });
    }
};

struct encrypt : private BaseCrypt { 
    encrypt(std::string const &key) : BaseCrypt(key) {}
    char operator()(unsigned char input) { 
        return xform<std::plus<char> >(input);
    }
};

struct decrypt : private BaseCrypt { 
    decrypt(std::string const &key) : BaseCrypt(key) {}
    char operator()(unsigned char input) { 
        return xform<std::minus<char> >(input);
    }
};

Some might disagree with my expanding such a simple encryption algorithm out to its component operations like I have in xform, but it's been my experience, that at least when you get to real encryption algorithms, it pays to be extremely explicit about everything (even at the expense of some verbosity, much as I usually dislike that), and generate all the intermediate values so 1) you can look at them when debugging the code, and 2) you can assure that intermediate values don't get extended out to larger sizes and such, which can produce code that works most of the time, but then fails inexplicably for some tiny fraction of possible inputs (years ago I spent something like three days on some DES code with exactly that sort of problem).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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