Pset2 - Vigenere - Encypts all variations of "barfoo" as "ca\..." (no case issues)

Question

This is my first question on Stack Exchange. I had a few different issues with this problem but I felt I'd finally sorted them out until I ran into this. I've been working for a number of hours today and feel like I may just be missing something obvious but I can't seem to pick it out.

Case and non-alpha characters don't seem to be an issue, and I can print out a lot of strings correctly with a lot of keys, but "barfoo" comes out as "ca" with a key of "baz," so my code gets three little frowny faces from Check50. It seems the issue is arising from the use of larger ASCII characters like "z" with other larger ASCII characters in the plaintext, but this problem isn't perfectly consistent.

Can anyone point out what I'm missing? Here's my code:

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

#define U_REDUX 65 //used to reduce uppercase ASCII letters to values of 0-25
#define L_REDUX 97 //same but with lowercase letters

int main(int argc, string argv[])
{
string pt; //plaintext
string key;
int keyLength;
int wrap = 0; //to increment/wrap around the key value when adding it to each char in pt

//if (more or less than two cmdln arguments) -> error
//else -> keyLength (used to wrap around) is set to length of argv[1]
if(argc != 2)
{
    printf ("Error 1\n");
    return 1;
} else {
    key = argv[1];
    keyLength = strlen(key);
}

//check each char in key
//if (char is not an alpha) -> error
//else -> shift char's numerical value according to case
for(int i = 0; i < keyLength; i++)
{
    if(!isalpha(key[i])) {
        printf("Error 2\n");
        return 1;
    } else if(isupper(key[i])) {
        key[i] = (key[i] - U_REDUX);
    } else {
        key[i] = (key[i] - L_REDUX);
    }
}

printf ("plaintext: ");
pt = get_string();

printf ("ciphertext: ");

//iterate over each char in pt and print it according to the key and case
//if char is not an alpha, print it as-is, without incrementing wrap
for(int i = 0; i < strlen(pt); i++)
{
    if(isalpha(pt[i])) {
            if(isupper(pt[i])) {
                pt[i] -= U_REDUX;
                pt[i] += key[wrap % keyLength];
                wrap++;
                pt[i] = pt[i] % 26;
                printf("%c", pt[i] + U_REDUX);
            } else {
                pt[i] -= L_REDUX;
                pt[i] += key[wrap % keyLength];
                wrap++;
                pt[i] = pt[i] % 26;
                printf("%c", pt[i] + L_REDUX);
        }
    } else {
        printf ("%c", pt[i]);
    }
}
printf ("\n");
}

Answer 1

The problem lies in the last for loop. The for loop is controlled by strlen(pt), which is recalculated on every loop iteration. Inside the loop, the ASCII values of the letters in pt are converted to numbers from 0 to 25, but are not converted back to encoded letters.

The effect is this. When an 'a' or 'A' is encoded, it is transformed to 0. It is actually stored as a single byte, unsigned int, or as 0x00. This happens to be the end of string marker, \0. When the loop comes back around, strlen(pt) depends on the end of string marker, so it will be recalculated differently. It will mistake the transformed 'a' as the end of string marker and the for loop exits.

Two solutions. Don't leave the contents of the string in the form of numbers from 0 to 25. Or, the better solution, calculate strlen(pt) once and store it. It won't change after the loop starts. This is the best practice:

        for(int i = 0, ptlen = strlen(pt); i < ptlen; i++)

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)