Incrementing and wrapping the key

Question

 string text = GetString();

//enters length of argv string into q
//converts string argv[1] into string key

string key = argv[1];
int klen = strlen(key);
int kposition = 0;


    //loop through the characters in array "text"
    for (int tposition = 0, n = strlen(text); tposition < n; tposition ++)
    {
        if isupper(key[kposition])
        {
            key[kposition] = ((key[kposition] - 'A') % klen) + 'A';
        }
        else if islower(key[kposition])
        {
            key[kposition] = ((key[kposition] - 'a') % klen) + 'a';
        }

        //determine if character is alphabetical
        if (isalpha(text[tposition])) 
        {

            //encrypt upper case characters
            if (isupper(text[tposition]))
            {
                //modulo magic to loop to beginning of alphabet after 'Z'
                text[tposition] = (((text[tposition] - 'A') + key[kposition]) % 26) + 'A';
                printf("%c", text[tposition]);
            }        
                //encrypt lower case characters
            else
            {
                //modulo magic to loop to beginning of alphabet after 'z'
                text[tposition] = (((text[tposition] - 'a') + key[kposition]) % 26) + 'a';
                printf("%c", text[tposition]);
            }
        }
        //if the input isn't alphabetical, then just print the input (spaces)
        else 
        {
            printf("%c", text[tposition]); 
        }
    kposition ++;

    }    
    printf("\n");
    return 0;

}

I followed your advice and made my variables a bit more descriptive, which helps me look at the code.

I am still not getting the correct outputs from my program, though it compiles and runs fine.

For example, when I run: vigenere.c -bacon

and enter: "Meet me at the park", I get: "Gxzq mr ni kyr frea", rather than the correct answer, which is: "Negh zf av huf pcfx".

So the upper/lower case is working, and the spaces are working. The problem lies with the incrementing of key[kposition]. Part of the problem is that I don't intuit modulo very well, so it's not clear to me exactly what the modulo arithmetic is doing (other than it gives the remainder of the two numbers).

How can I arrange my key[kposition] incrementor better?

Answer 1

That inner for loop, incrementing j, seems completely out of place and unneeded to me.

What you're doing first looks pretty good already: Define two indices for iteration, one over the plaintext (i) and one over the key (j).

Then you just need to iterate over the plaintext. You encrypt the character at plaintext position i with the character at key position j. Then you increase both iteration indices by one, keeping the key index in range (by using modulo for example).

So, basically you just need to get rid of that inner for loop and add logic to increment j with the outer loop.

Concerning your code: It's advisable to split it up a bit more: Write a function to encrypt a single character and use it in your loop. Use better names for your variables. i and j aren't descriptive.

Answer 2

To debug this yourself, print out the values of key[kposition]

You need to limit kposition

kposition = (kposition+1)%klen;

and only modify it for alphabetic characters.

Have the current key as a temp, don't re-add the 'A', which doesn't quite do modulo 26 correctly

key[kposition] = ....;

Goes to

Tkey = ((key[kposition] - 'a') );

or

Tkey = ((key[kposition] - 'A') );

You don't mod klen, and don't update key. Change other key[kposition] to Tkey

#include <stdio.h>
#include <string.h>

int main( int argc, char ** argv)
{
    char  text[] = "Meet me at the park";
    char * key = "bacon";

//enters length of argv string into q
//converts string argv[1] into string key

//string key = argv[1];
int klen = strlen( key);
int kposition = 0;


//loop through the characters in array "text"
for (int tposition = 0, n = strlen( text); tposition < n; tposition++)
{
    unsigned Tkey = 0;
    if( isupper(key[kposition]) )
    {
        Tkey = ((key[kposition] - 'A') );
    }
    else if( islower(key[kposition]) )
    {
        Tkey = ((key[kposition] - 'a') );
    }

    //determine if character is alphabetical
    if (isalpha(text[tposition]))
    {

        //encrypt upper case characters
        if (isupper(text[tposition]))
        {
            //modulo magic to loop to beginning of alphabet after 'Z'
            text[tposition] = (((text[tposition] - 'A') + Tkey) % 26) + 'A';
            kposition = (kposition + 1) % klen;

            printf("%c", text[tposition]);
        }
        //encrypt lower case characters
        else
        {
            //modulo magic to loop to beginning of alphabet after 'z'
            text[tposition] = (((text[tposition] - 'a') + Tkey) % 26) + 'a';
            kposition = (kposition + 1) % klen;
            printf("%c", text[tposition]);
        }
    }
    //if the input isn't alphabetical, then just print the input (spaces)
    else
    {
        printf("%c", text[tposition]);
    }

}
printf("\n");
return 0;
}