Why is my vigenere.c not working?

Question

I keep making changes to the looping part of this code and my check50 always fails. I don't know what's going on. Below is my code:

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

int main(int argc, string argv[])
{
    // declare variables
    int cipherText;

    if (argc != 2)
    {
        printf("Usage: ./vigenere keyword");
        printf("\n");
        return 1;
    }
    // keyword is the second command line argument
    string key = argv[1];
    int keylen = strlen(argv[1]);

    // iterate through keyword to check if alphabetical
    for (int i = 0, n = strlen(argv[1]); i < n; i++)
    {
        if ((key[i] >= '0') && (key[i] <= '9'))
        {
            printf("Keyword must consist only of letters.");
            return 1;
        }
    }

    // get the plaintext
    string plainText = GetString();

    // encypher - iterate over the characters in string, print each one encrypted
    for (int i = 0, j = 0, n = strlen(plainText); i < n; i++, j++)
    {
        // start the key again if key shorter than plainText
        if (j >= strlen(key))
        {
            j = 0;
        }

        // skip key[j] if plainText[i] is not an alpha character
        if (!isalpha(plainText[i]))
        {
            j = (j-1);
        }

        // makes Aa = 0, Zz = 25 for the uppercase letters
        if (isupper(key[j]))
        {
            key[j] = (key[j] - 'A');
        }

        // makes Aa = 0, Zz = 25 for lowercase letters
        else if (islower(key[j]))
        {
            key[j] = (key[j] - 'a');
        }


        if (isupper(plainText[i]))
        {
            cipherText = (plainText[i] - 'A');
            cipherText = ((cipherText + key[j%keylen])%26) + 'A';
            printf("%c", cipherText);
        }

        else if (islower(plainText[i]))
        {
            cipherText = (plainText[i] - 'a');
            cipherText = ((cipherText + key[j%keylen])%26 + 'a');
            printf("%c", cipherText);
        }

        else 
        {
            printf("%c", plainText[i]);
        }   
    }
    printf("\n");
    return 0;

}

Some answered this: "The first for loop has a problem. The condition is checking for i > keylen when it should be checking for i < keylen".

Also when computing the next output value, the steps should be

• (p[i]-65) results in a number between 0 and 25
• adding (key[i % keylen]) results in a number between 0 and 50
• apply modulo 26 so the number is between 0 and 25 (this is the missing step)
• then add 65 to get the output"

and it's what I tried to do.

Answer 1

Given this code:

int keylen = strlen(argv[1]);

// iterate through keyword to check if alphabetical
for (int i = 0, n = strlen(argv[1]); i < n; i++)
{
    if ((key[i] >= '0') && (key[i] <= '9'))
    {
        printf("Keyword must consist only of letters.");
        return 1;
    }
}

Your test inside the loop identifies digits as 'not a letter' (which is valid), but ignores punctuation, spaces and so on. You should probably be using if (!isalpha(key[i])) for the test (and it is courteous to print the erroneous character in the error message, which should be printed on standard error, not standard output, and should end with a newline:

        fprintf(stderr, "Keyword must consist only of letters (%c found at %d)\n",
                key[i], i+1);

You could refine that so it doesn't try printing non-printable characters with %c, but this is a huge step in the right direction.

You really don't need to set n in the loop; you just set keylen before the loop, so you could have written:

for (int i = 0; i < keylen; i++)

However, that is mostly cosmetic. Your real problem lies here:

    // start the key again if key shorter than plainText
    if (j >= strlen(key))
    {
        j = 0;
    }

    // makes Aa = 0, Zz = 25 for the uppercase letters
    if (isupper(key[j]))
    {
        key[j] = (key[j] - 'A');
    }

    // makes Aa = 0, Zz = 25 for lowercase letters
    else if (islower(key[j]))
    {
        key[j] = (key[j] - 'a');
    }

You modify the key string on each iteration through the key. Unfortunately, though, if any of the letters in the key is a or A, you've converted that to '\0', which means that strlen(key) returns a different answer from before. So, you should use keylen in place of strlen(). AFAICS, if there isn't an a or A, that part of the code is OK.

Later, you have:

    if (isupper(plainText[i]))
    {
        cipherText = (plainText[i] - 'A');
        cipherText = ((cipherText + key[j%keylen])%26) + 'A';
        printf("%c", cipherText);
    }

The j % keylen is superfluous; j is already limited to 0 .. keylen-1. Similarly with the code for lower-case text.

Putting these changes together, and dummying up a GetString() function using fgets(), I get:

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

typedef char *string;

static char *GetString(void)
{
    static char buffer[4096];
    if (fgets(buffer, sizeof(buffer), stdin) == 0)
    {
        fprintf(stderr, "EOF detected in GetString()\n");
        exit(EXIT_SUCCESS);
    }
    buffer[strlen(buffer) - 1] = '\0';
    return buffer;
}

int main(int argc, string argv[])
{
    // declare variables
    int cipherText;

    if (argc != 2)
    {
        printf("Usage: ./vigenere keyword");
        printf("\n");
        return 1;
    }
    // keyword is the second command line argument
    string key = argv[1];
    int keylen = strlen(argv[1]);

    // iterate through keyword to check if alphabetical
    for (int i = 0; i < keylen; i++)
    {
        if (!isalpha(key[i]))
        {
            printf("Keyword must consist only of letters (%c at %d)\n",
                   key[i], i+1);
            return 1;
        }
    }

    // get the plaintext
    string plainText = GetString();

    // encypher - iterate over the characters in string, print each one encrypted
    for (int i = 0, j = 0, n = strlen(plainText); i < n; i++, j++)
    {
        // start the key again if key shorter than plainText
        if (j >= keylen)
        {
            j = 0;
        }

        // skip key[j] if plainText[i] is not an alpha character
        if (!isalpha(plainText[i]))
        {
            j = (j - 1);
        }

        // makes Aa = 0, Zz = 25 for the uppercase letters
        if (isupper(key[j]))
        {
            key[j] = (key[j] - 'A');
        }
        // makes Aa = 0, Zz = 25 for lowercase letters
        else if (islower(key[j]))
        {
            key[j] = (key[j] - 'a');
        }

        if (isupper(plainText[i]))
        {
            cipherText = (plainText[i] - 'A');
            cipherText = ((cipherText + key[j]) % 26) + 'A';
            printf("%c", cipherText);
        }
        else if (islower(plainText[i]))
        {
            cipherText = (plainText[i] - 'a');
            cipherText = ((cipherText + key[j]) % 26 + 'a');
            printf("%c", cipherText);
        }
        else
        {
            printf("%c", plainText[i]);
        }
    }
    printf("\n");
    return 0;
}

Sample run:

$ ./vigenere bakedalaska
What a wonderful world! The news is good, and the Vigenere cipher is solved.
Xhkx d wznvorguv arrwd! Lre oegw ls rogn, aod dlh Vtgwxese mmshpr ac splfig.
$