encrypts "a" as "b" using 1 as key output not valid ASCII text

Question

I have been trying to figure out what the issue is in my Caesar code. When I run check50, I see 2 errors -

encrypts "a" as "b" using 1 as key output not valid ASCII text

&

encrypts "world, say hello!" as "iadxp, emk tqxxa!" using 12 as key output not valid ASCII text

Could someone please help? I could not figure out from the previous answers. Thank you very much!

#include <cs50.h>
#include <stdio.h>
#include <ctype.h>
#include <string.h>
#include <stdlib.h>
int main(int argc, string argv[])
{
    //print message if argc count is not 2 & argv[1] is not present
    if ((argc != 2) || (argv[1] == NULL))
    {
        printf("Usage: ./caesar key\n");
        return 1;
    }
    string key = argv[1];
    //check every letter of the argv[] string to see if that is a digit
    for (int i = 0; i < strlen(key); i++)
    {
        if (!isdigit(key[i]))
        {
            printf("Usage: ./caesar key\n");
            return 1;
        }
    }
    int k = atoi(argv[1]);
    //printf("key is %i\n", k);
    string p = get_string("plaintext:  ");
    int plen = strlen(p);
    char c[plen];
    //Loop though plaintext to convert each character to secret character
    for (int i = 0; i < strlen(p); i++)
    {
        if ((isalpha(p[i])) && (isupper(p[i])))
        {
            c[i] = (((p[i] - 'A') + k) % 26) + 'A';
        }
        else if ((isalpha(p[i])) && (islower(p[i])))
        {
            c[i] = (((p[i] - 'a')+ k) % 26) + 'a';
        }
        else
        {
            c[i] = p[i];
        }
    //printf("ciphertext: %s\n", c);
    }
    printf("ciphertext: %s", c);
    printf("\n");
    return 0;
}

Answer 1

Quick question: At what point does this code place the end of string marker, \0 at the end of the string in string c? Oh, and is there room for it?

Programming tips: The tests for isalpha are redundant and unnecessary. If testing for isupper or islower, the code is automatically testing for isalpha.

When setting up a for loop like for (int i = 0; i < strlen(p); i++), best practice is to get the string length, store it in a variable, and use the var in the for loop, like this:

int len_p = strlen(p); i++);
for (int i = 0; i < len_p; i++)

There are two reasons for this. First, this means that the string length needs only be calculated once, rather than every loop pass. Second, under some conditions, a char in the string could be reassigned to 0x00, which is the end of string marker, and will cause the loop to end prematurely.

Finally, avoid the use of single letter variable names. If you ever have to come back later and edit code, finding every occurrence of a single letter variable is nearly impossible. (Try searching your code for every occurrence of c.) Instead, get in the habit now of using descriptive var names. The only accepted use of single letter vars, are the typical use of i, j, etc. in for loops. Even then, only for short loops. If they get too long, descriptive var names should be used.

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