Pset2 - Vigenere looping problem

Question

My code works properly for the Vigenere problem, but only if the key and the plaintext have the same number of letters in them. I think the problem is in the loops I'm trying to implement, but I'm not sure where to go from here.

Here's the code:

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

int main (int argc, string argv [])
{

    if (argc !=2)
    {
        printf ("Error. Instructions not followed.\n");
        return 1;
    }

    string key = argv[1];
    for (int i = 0, n = strlen (key); i < n; i++)
    {
        if (!isalpha(key[i]))
        {
            printf ("Non-alphabetical characters.\n");
            return 1;
        }
    }
    printf ("plaintext: ");
    string plaintext = get_string();
    printf ("%s\n", plaintext);
    printf("ciphertext:");
    for (int g = 0, k = 0, z = strlen (plaintext), v = strlen (key); g < v, k < z; g++, k++)
    {
        if (isalpha(plaintext[k]) && isalpha(key[g]))
        {
            if (isupper(plaintext[k]) && isupper(key[g]))
            {
                printf("%c", (((((plaintext[k])-65)+(key[g]-65))%26)+65));
            }
        }
    }
}

Any help would be appreciated. Thanks in advance.

Answer 1

Your for loop syntax is not likely doing what you think it is.

for (int g = 0, k = 0, z = strlen (plaintext), v = strlen (key); g < v, k < z; g++, k++)

Comma operators are sometimes used in the first part of a for loop, when you want two initialization steps to take place before the loop starts.

Comma operators are sometimes used in the last part of a for loop, when you want two counters incremented at the end of each loop iteration.

However, they are rarely used in the middle expression. The comma operator

evaluates its first operand and discards the result, and then evaluates the second operand and returns this value.

In your loop, it will test g < v, and then throw the result away... and test k < z. If k < z the loop will continue. I don't know how you got this code to compile in the CS50 IDE. If you use the default make command, you'd get the following error:

error: relational comparison result unused [-Werror,-Wunused-comparison]

which is the compiler's attempt to tell you that the g < v comparison is being discarded.

If you want to make two comparisons in a for loop, you need to use a compound comparison (like you did in the if loops):

for (int g = 0, k = 0, z = strlen (plaintext), v = strlen (key); g < v && k < z; g++, k++)

This STILL won't completely solve your problem, because now the loop will exit when you run out of characters in key. You should only be testing to make sure k is in bounds for plaintext. Instead of incrementing g every time through the loop, you should be incrementing it once every time you use one of the key's characters (and then using modulus to wrap it around if it runs out of characters). I would try starting that loop from scratch and try to use printf statements just to step through plaintext and key correctly before worrying about the actual encryption.

As an aside, for your own sanity while debugging, I would highly recommend against putting that many expressions into a for loop header. One of the dangers (and ironically... greatest strengths) of C is that the compiler will pretty much let you do almost anything without complaining too much. However, the more things that can go wrong on one line, the harder it is to discover where a mistake really is.