0

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;
}



1 Answer 1

0

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. ;-)

2
  • Thanks Cliff! Using descriptive variable names instead of the single variable letter, using a variable for string length made sense to me. But I still couldn't fully understand your 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?". Then I Googled, rewatched the String, Nul character section in the week 2 video. Then finally understood that my for loop is missing the end of string nul check by not including the <=. Viola! That fixed this issue that I have been banging my head on for 2 weeks now :) Feb 21 at 18:14
  • Well, you haven't fixed it completely. There's still a potentially fatal bug in the code. Look at the declaration char c[plen]; It allocates space for the string but not the end of string marker. When you copy the end of string marker, the code is actually writing that EOS marker to the first byte following the string. This overwrites the first byte of whatever memory is physically stored next in memory. It's absolutely necessary to allocate that extra byte, like this: char c[plen+1];
    – Cliff B
    Feb 21 at 18:47

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .