Vigenere CS50 - Need help cycling through alpha letters

I'm trying to do the CS50 Vigenere exercise.

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

int main(int argc, string argv[])
{
//Check for 2 command line arguments
if (argc != 2)
{
    printf("Nah bro, you gotta have 2 arguments.\n");
    return 1;
}
//Check is alpha
else {
    for (int i = 0; i < strlen(argv[1]); i++)
    {
        if (isalpha(argv[1][i]) == 0)
        {
            printf("Nah bro, u gots to use letters.\n");
            return 1;
        }
    }
}


//Prompt user to input text
    printf("plaintext: ");
    string p = get_string();

//Cipher
    printf("ciphertext: ");
    string k = argv[1];
    int cipherlen = strlen(k);



//Cycle through key letters
for (int i = 0, j = 0, n = strlen(p); i < n; i++)
{


    if (isalpha(p[i]))
    {

        if (isupper(p[i]))
            {
            printf("%c", ((p[i] - 65) + (k[(j % cipherlen)]) - 65) % 26 + 65);
            j++;
            }


        else if (islower(p[i]))
            {
            printf("%c", ((p[i] - 97) + (k[(j % cipherlen)]) - 97) % 26 + 97);
            j++;
            }


        else
            printf ("%c", p[i]);
    }

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

Here are my error codes according to the check:

https://cs50.me/checks/a56bc9325327035cb0e8d831693c9805c4b6468b

I understand my problem has to do with cycling through each letter but not applying it to spaces or symbols. I've tried using an if (isalpha) statement and an else printf(" ") but it doesn't work well for numbers or symbols. I figured adding j++ would iterate only through alpha characters but it doesn't seem to help.

Is there something here super plain I'm missing?

Solution

The basic structure of your code looks OK.

I see three problems with it:

All of your printfs are guarded by the if (isalpha(p[i])) check, so your program never outputs anything if the plaintext character is not alphabetic (it should output the character unchanged instead). The fix for this is simple; just remove the outer if (...) in the loop:

for (int i = 0, j = 0, n = strlen(p); i < n; i++)
{
    if (isupper(p[i]))
        {
        printf("%c", ((p[i] - 65) + (k[(j % cipherlen)]) - 65) % 26 + 65);
        j++;
        }
    else if (islower(p[i]))
        {
        printf("%c", ((p[i] - 97) + (k[(j % cipherlen)]) - 97) % 26 + 97);
        j++;
        }
    else
        printf ("%c", p[i]);
}

The inner if/else if chain handles this case correctly.

The current plaintext character p[i] and the current keyword character k[j % cipherlen] can be uppercase / lowercase independently. Your code currently does not handle this at all; instead it assumes that if p[i] is uppercase, k[j % cipherlen] must be uppercase as well, and similarly for lowercase.

By the way, I recommend against writing 65 and 97 in the code. I'd use 'A' and 'a' instead, respectively, which makes things more readable IMHO.

To fix this issue, you have to test k[j % cipherlen] for uppercase / lowercase separately. For example:
```
for (int i = 0, j = 0, n = strlen(p); i < n; i++)
{
    char key_char = k[j % cipherlen];
    int key_shift;
    if (isupper(key_char)) {
        key_shift = key_char - 'A';
    } else {
        key_shift = key_char - 'a';
    }
    if (isupper(p[i]))
        {
        printf("%c", ((p[i] - 'A') + key_shift) % 26 + 'A');
        j++;
        }
    else if (islower(p[i]))
        {
        printf("%c", ((p[i] - 'a') + key_shift) % 26 + 'a');
        j++;
        }
    else
        printf ("%c", p[i]);
}
```
(I got tired of repeatedly typing the same expressions, so I extracted the common bits out to variables (key_char, key_shift). The only tricky part here is that j should only be incremented if key_shift is actually used, but your code already handles that.)
This is a subtle point, but all the <ctype.h> functions (such as isupper, isalpha, ...) have undefined behavior if the argument is negative. char is a signed type in many implementations, so a random character str[i] may well be negative. To be completely portable and correct, you should cast the character to (unsigned char) in each such call:
```
if (isupper((unsigned char)key_char))
    ...
if (isupper((unsigned char)p[i]))
    ...
else if (islower((unsigned char)p[i]))
    ...
```
Alternatively, just fully embrace ASCII (the rest of your code assumes it already) and do:
```
if (key_char >= 'A' && key_char <= 'Z')
    ...
if (p[i] >= 'A' && p[i] <= 'Z')
    ...
else if (p[i] >= 'a' && p[i] <= 'z')
    ...
```