Crashing Vigenere Cipher program

I made a program to encode and decode messages using a Vigenere Cipher and a key. (if you dont know what a Vigenere Cipher is... then check Wikipedia.) It should work *fine*, but it keeps hanging at the point when it should show the encrypted message/decrypted message.

Code:

---

#include<iostream>
#include<windows.h>
using namespace std;
class Frame
{
 public:
        char square();
        int enc();
        int dec();
};
char Frame::square()
{
    // ===(Vigenere Square Generator)===
    char a[26][26], alpha='a', flag='a', showvig;
    for(int col=0; col<26; col++)
    {
            for(int row=0; row<26; row++)
            {
                    a[col][row]=alpha;              //'a[][]' is assigned alpha's value
                    alpha++;                        //Alpha increased
                    if(alpha>'z')
                                alpha='a';          //continues the series after z
//                  cout<<a[col][row]<<" ";          //optional display statement
            }
            flag++;                                  //Value of flag increased
            alpha=flag;                              //Value of flag assigned to a
    }
/*    cout<<"\nDisplay Vigenere Square?(Y/N) ";
    cin>>showvig;
    if(showvig=='Y' || showvig=='y')
    {
        for(int col=0; col<26; col++)
        {
            for(int row=0; row<26; row++)
            {
                cout<<a[col][row]<<" ";
            }
            cout<<endl;
        }
    } */
    return a[26][26]; 
}
int Frame::enc()
{ 
    char b[26][26];
    b[26][26]=square();
    // ===(ACME Encrypt O Matic)===
    char key[50], srchk, srchm, message[100], encoded[100];
    int keylen, msglen, arow, bcol, k;
    cout<<"\nEncrypter ready";
    cout<<"\nInsert key... ";
    cin.getline(key,50);
    cin.getline(key,50);
    cout<<"\nShowing the key..... "<<key<<key<<key;  //Comparison purposes
    cout<<"\nEnter the message... ";
    cin.getline(message,100);
    keylen=strlen(key);
    msglen=strlen(message);
    cout<<"\nEncrypted message... ";
    for(int i=0; i<msglen; i++)
    {
            k=i;                                    //Allows for the messages
            if(i>keylen)                            //to be longer than the
                        k=0;                        //key ie repeating key behaviour
            srchk=key[k];
            srchm=message[i];
            if(message[i]==' ')
            {
                        encoded[i]=' ';
                        goto label;
            }
            for(int col=0, row=0; col<26; col++)
            {
                    if(srchk==b[row][col])
                    {
                                          bcol=col;
                                          break;
                    }
            }
            for(int row=0, col=0; row<26; row++)
            {
                      if(srchm==b[row][col])
                      {
                                            arow=row;
                                            break;
                      }
            }
            encoded[i]=b[arow][bcol];
            label:
            cout<<encoded[i];
    }
    cout<<endl;
    system("PAUSE");
    return 0;
}
int Frame::dec()
{   
    char c[26][26];
    c[26][26]=square();
    // ===(ACME Encryption-Be-Gone)===
    char key[50], srchk, srchm, message[100], encoded[100];
    int keylen, msglen, arow, bcol, k;
    cout<<"\nInitializing...";
    Sleep(5);
    cout<<"\nDecrypter ready";
    cout<<"\nInsert key... ";
    cin.getline(key,50);
    cin.getline(key,50);
    cout<<"\nShowing the key..... "<<key<<key<<key;  //Comparison purposes
    cout<<"\nEnter coded mssg.... ";
    cin.getline(encoded,100);
    keylen=strlen(key);
    msglen=strlen(encoded);
    cout<<"\nDecrypted message... ";
    for(int i=0; i<msglen; i++)
    {
            k=i;                                    //Allows for the messages
            if(i>keylen)                            //to be longer than the
                        k=0;                        //key ie repeating key behaviour
            srchk=key[k];
            srchm=encoded[i];
            if(encoded[i]==' ')
            {
                        message[i]=' ';
                        goto label2;
            }
            for(int col=0, row=0; col<26; col++)
            {
                    if(srchk==c[row][col])
                    {
                                          bcol=col;
                                          break;
                    }
            }
            for(int row=0, col=bcol; row<26; row++) //col=0 changed
            {
                        if(srchm==c[row][col])
                        {
                                col=0;
                                bcol=col;
                                arow=row;
                                break;
                        }
            }
            message[i]=c[arow][bcol];
            label2:
            cout<<message[i];
    }
    cout<<endl;
    system("PAUSE");
    return 0;
}
int main()
{
    Frame ob;
    int ch;
    cout<<"\n(1)Encrypt or (2)decrypt?";
    cin>>ch;
    if(ch==1)
            ob.enc();
    else
        if(ch==2)
                ob.dec();
    return 0;
}

---

What exactly is t3h problem? It worked fine when I ran the encrypt/decrypt sections separately (by putting all the code in just the main(), and putting comments on the encrypt or decrypt sections)

-regards,
ultrabot90

>> return a[26][26];
You can't return arrays like that. Instead of returning the array, make a parameter for the function that is char a[26][26] and remove the local a array. Then just pass in the c array when you call the function and it will get updated appropriately.

It seems to me (after trying out your code) that you use variables that are undefined, e.g.:

Code:

---

                        encoded[i]=b[arow][bcol];

---

arow is some huge value (Perhaps I'm doing something wrong in using the code tho').

I'm also pretty sure that you need to do this differently:

Code:

---

                k=i;                                    //Allows for the messages
                if(i>keylen)                            //to be longer than the
                        k=0;                        //key ie repeating key behaviour
                srchk=key[k];

---

If you give "KEY" as the key, then after the third letter, it will just repeat "KKKKKKKKKKKKK" until the end of the message, which is of course not particularly good encryption (could just as well use cesars cipher in that case).

You probably want to do:

Code:

---

                k++;                                    //Allows for the messages
                if(k>keylen)                            //to be longer than the
                        k=0;                        //key ie repeating key behaviour
                srchk=key[k];

---

--
Mats

Also

Code:

---

        return a[26][26];

---

That returns the 27th column of the 27th row! - You only have 26 of each.

Of course, looking further at your code, it seems like you actaully want to return the FULL 2D array.

--
Mats

@Daved - That indeed seems to be the problem...I've edited it, though I dunno if I did it right...

Code:

---

public:
        void square();
        int enc(char[26][26]);
        int dec(char[26][26]);
};

---

Code:

---

void Frame::square()
{
    // ===(Vigenere Square Generator)===
    char a[26][26], alpha='a', flag='a', showvig;

---

Code:

---

int Frame::enc(char b[26][26])
{ 
    // ===(ACME Encrypt O Matic)===
    char key[50], srchk, srchm, message[100], encoded[100];

---

Code:

---

int Frame::dec(char b[26][26])
{   
    // ===(ACME Encryption-Be-Gone)===

---

Code:

---

int main()
{
    Frame ob;
    int ch;
    cout<<"\n(1)Encrypt or (2)decrypt?";
    cin>>ch;
    if(ch==1)
            ob.enc(char[][]);
    else
        if(ch==2)
                ob.dec(char[][]);
    return 0;
}

---

In the main(), however, it gives errors for both of the enc() and dec() functions, that "expected primary-expression before "char"". Now what do I do? :(

@matsp- Omg no, that part is correct, 100%! I've already said that it works fine when running individual encode/decode functions.
On the key repeating lines...
The key is supposed to be written on top of the message, again and again, so that every letter of the message matches up with a key letter it is to be encoded with. Hence, both key and message run with [i], simultaneously, on untill the message to be encoded/decoded ends (in key[k] I made an exception, lets see why ->). As soon as i becomes greater than the key length, k resets to 0, and since key is working according to k, it repeats...Simple! ^_^

arow and bcol are just used to store the 'coordinates' of the required character which is to be input into the encoded/decoded message. arow and bcol arent huge, they're both less than 26 xD
EDIT - Omg the second thing hell thats true it'd return a[26][26]th element! Sheesh...
-regards,
ultrabot90

You got it backwards. You want to have square take the parameter and the other functions pass in the arrays they create.

Why not leave the array inside the class as a private member variable, and save all this messing about passing the array in and out every time you want to do something.

And can I just say "ugh" to the goto's.

Gotos should go... Especially since the goto is fairly easy to get rid of by just using an "else" at the end of the if which contaisn the goto (and of course braces around the code down to the line before the now defunct label).

I also agree on the use of square as a member of the class.

By the way, your code only works right if the key and message are lower-case. Maybe you want to add

Code:

---

                srchk=tolower(key[k]);

---

and corresponding on srchm.

If you add the red bit:

Code:

---

        cin>>ch;
        cin.ignore();


---

you don't have to do two getline's (because the "ignore" bit will "eat" the newline that is otherwis sitting there and making key an empty string.

Of course, it's all broken for non-alpha characters, so perhaps your lines

Code:

---

                if(encoded[i]==' ')
                {
                        message[i]=' ';

---

should be:

Code:

---

                if(!isalpha(encoded[i]))
                {
                        message[i]= encoded[i];

---

And of course the corresponding code in encode.

The for-loops that you use for searching in the encode can be easily replaced with simple math:

Code:

---

    bcol = srchk - 'a';
    arow = srchm - 'a';

---

.
And the bcol can be done the same way in the decode part.


--
Mats

Quote:

---

Originally Posted by Salem

Why not leave the array inside the class as a private member variable, and save all this messing about passing the array in and out every time you want to do something.

---

Yeah, that's something I would also suggest, have the table exist as a private data member. And it looks like you recreate the table each time you encrypt/decrypt something. Do it once (put the code that builds the table in the constructor for example) and access that table directly from the enc and dec functions, no need to pass it around anymore. You can ditch the square function then as well.

Code:

---

class Frame
{
    char a[26][26];
 public:
    //char square();  // Ditch this
    Frame();  // Add this
    int enc();
    int dec();
};

Frame::Frame()  // Builds the table
{
    for( int col=0; col<26; ++col )
        for( int row=0; row<26; ++row )
            a[col][row] = 'a' + ((col+row)%26);
}

---

Then you can get rid of the b and c arrays in enc and dec and just use the a array everywhere instead.

Omg, ideas nothing short of genius, specially that

Code:

---

    bcol = srchk - 'a';
    arow = srchm - 'a';

---

part. And hey, how about putting the key, srchk, srchm, message, encoded, keylen, msglen, arow and bcol in the private section? Its down from ~150 lines to 99...cool.
Now, in sync with the newly learnt stuff...

Code:

---

#include<iostream>
#include<windows.h>
using namespace std;
class Frame
{
 private:
        char a[26][26], key[50], srchk, srchm, message[100], encoded[100];
        int keylen, msglen, arow, bcol, k;
 public:
        Frame()
        {
              for( int col=0; col<26; ++col )
                    for( int row=0; row<26; ++row )
                        a[col][row] = 'a' + ((col+row)%26);
        }
        int enc();
        int dec();
};
int Frame::enc()
{ 
    // ===(ACME Encrypt O Matic)===
    cout<<"\nEncrypter ready ";
    cout<<"\nInsert key... ";
    cin.getline(key,50);
    cout<<"\nShowing the key..... "<<key<<key<<key;
    cout<<"\nEnter the message... ";
    cin.getline(message,100);
    keylen=strlen(key);
    msglen=strlen(message);
    cout<<"\nEncrypted message... ";
    for(int i=0; i<msglen; i++)
    {
            k=i;
            if(i>keylen)
                        k=0;
            srchk=tolower(key[k]);
            srchm=tolower(message[i]);
            if(!isalpha(message[i]))
                        encoded[i]=message[i];
            else
            {
                bcol = srchk - 'a';
                arow = srchm - 'a';
                encoded[i]=a[arow][bcol];
            }
            cout<<encoded[i];
    }
    cout<<endl;
    system("PAUSE");
    return 0;
}
int Frame::dec()
{   
    // ===(ACME Encryption-Be-Gone)===
    cout<<"\nDecrypter ready ";
    cout<<"\nInsert key... ";
    cin.getline(key,50);
    cout<<"\nShowing the key..... "<<key<<key<<key;
    cout<<"\nEnter coded mssg.... ";
    cin.getline(encoded,100);
    keylen=strlen(key);
    msglen=strlen(encoded);
    cout<<"\nDecrypted message... ";
    for(int i=0; i<msglen; i++)
    {
            k=i;
            if(i>keylen)
                        k=0;
            srchk=tolower(key[k]);
            srchm=tolower(encoded[i]);
            if(!isalpha(encoded[i]))
                                    message[i]= encoded[i];
            else
            {
                bcol = srchk - 'a';
                arow = srchm - 'a';
                message[i]=a[arow][bcol];
            }
            cout<<message[i];
    }
    cout<<endl;
    system("PAUSE");
    return 0;
}
int main()
{
    Frame ob;
    int ch;
    cout<<"\n(1)Encrypt or (2)Decrypt?";
    cin>>ch;
    cin.ignore();
    if(ch==1)
            ob.enc();
    else
        if(ch==2)
                ob.dec();
    return 0;
}

---

Guess what...it still crashes. What could possibly be the problem? -_-

Doesn't crash on my machine. What do you do to crash it?

--
Mats

Quote:

---

Originally Posted by matsp

Doesn't crash on my machine. What do you do to crash it?

--
Mats

---

Dunno, I run encode, put in a short (7 letter) key, type in a message usually being shorter than the key, and it crashes. Machine is Intel P4 1.72 GHz, 256 MB SDRAM, atleast 12/40 gb HDD space free, running Windows XP Pro SP2, onboard 32 mb gfx card. Do I need to give the error details (!!!) next? o.o

What key do you give it? If you don't want to post the key you're using, find another that causes the crash that you can post.

It ran, but with decryption errors...
With key as "systemofadown" it encrypts "omfg wtf" as "gkxz ihk"...but decrypts the same as "yips uvp".

Yes, you are doing "encryption" in the decryption part. Unfortunately, [I don't think, at least] you can't remove the loop bit out of the decode part.

Replacing the

Code:

---

arow = srchm - 'a'

---

with:

Code:

---

                                for(int row=0; row<26; row++) //col=0 changed
                                {
                                                        if(srchm==a[row][bcol])
                                                        {
                                                                        arow=row;
                                                                        break;
                                                        }
                                }

---

gives this:

Code:

---

(1)Encrypt or (2)Decrypt?1

Encrypter ready
Insert key... hello

Showing the key..... hellohellohello
Enter the message... abc

Encrypted message... hfn

(1)Encrypt or (2)Decrypt?2

Decrypter ready
Insert key... hello

Showing the key..... hellohellohello
Enter coded mssg.... hfn

Decrypted message... abc

---

--
Mats

How about this for an improvement:
1: Make the encode/decode functions take the key and text to encode/decode as string arguments, rather than ask for them inside the function.

Code:

---

        string enc(const string &key, const string &message);
        string dec(const string &key, const string &encoded);

---

I'm intentionally leaving the code inside those two member functions out, since I think it would be good practice to rewrite your function [it's actually pretty small changes].

This of course means that the code to read the key and message will have to move elsewhere - I changed main to look like this:

Code:

---

int main()
{
    Frame ob;
    int ch;
        int done = 0;
        char text[100];
        string key;
        string result;
        while(!done)
        {
                cout<<"\n(1)Encrypt or (2)Decrypt?";
                cin>>ch;
                cin.ignore();

                switch(ch)
                {
                case 1:
                    cout<<"\nInsert key... ";
                        cin >> key;
                        cin.ignore();
                        cout<<"\nEnter message.... ";
                        cin.getline(text,100);
                        result = ob.enc(key, text);
                        cout << "encrypted: \"" << result << "\"." << endl;
                        break;
                case 2:
                    cout<<"\nInsert key... ";
                        cin >> key;
                        cin.ignore();
                        cout<<"\nEnter coded msg.... ";
                        cin.getline(text,100);
                        result = ob.dec(key, text);
                        cout << "decrypted: \"" << result << "\"." << endl;
                        break;
                case 0:
                        done = 1;
                        break;
                }
        }
    return 0;
}

---

2: Use modulo to calculate the current key-character:

Code:

---

            k=i % keylen;
            srchk=tolower(key[k]);

---

This replaces this code:

Code:

---

            k=i;
            if(i>keylen)
                        k=0;
            srchk=tolower(key[k]);

---

It is shorter and more succinct, but on the other hand, it takes a few more clock-cycles (because it uses divide, which is one of the "worst" instructions you can give to any processor).

On the other hand, this change cleans up the class and perhaps gives a (slight) performance enhancement: Move these variable declarations of these things back into the enc/dec functions:

Code:

---

        int keylen, msglen, arow, bcol, k;
        char srchk, srchm;

---

There is no need for them to be part of the class, and if you are unlucky, the code will be slower because the compiler has to access them through the "this" pointer, and certainly, the compiler can't be as agressive when it comes to not storing the final result of these. This is particularly meaningless as enc/dec never use the value from one call to another, so there's absolutely no need to store back any of it into the class - but it's very hard for the compiler to figure that part out, since the compiler doesn't quite follow all that.

A class should only have member components that NEED to be in the class for it's functionality. Temporary variables that are only used within a member function shouldn't be part of the class.

[And the above change to add parameters to enc/dec means that key, message and encoded should also be removed from the class].


--
Mats