CS50 Caesar program is working but check50 says it isn't

Question

I created this program but I'm getting errors on CS50 showing that I didn't do any of it correctly.

The requirements are as follows:

Implement your program in a file called caesar.c in a directory called caesar.

Your program must accept a single command-line argument, a non-negative integer. Let’s call it k for the sake of discussion.

If your program is executed without any command-line arguments or with more than one command-line argument, your program should print an error message of your choice (with printf) and return from main a value of 1 (which tends to signify an error) immediately.

If any of the characters of the command-line argument is not a decimal digit, your program should print the message Usage: ./caesar key and return from main a value of 1.

Do not assume that k will be less than or equal to 26. Your program should work for all non-negative integral values of k less than 2^31 - 26. In other words, you don’t need to worry if your program eventually breaks if the user chooses a value for k that’s too big or almost too big to fit in an int. (Recall that an int can overflow.) But, even if k is greater than 26, alphabetical characters in your program’s input should remain alphabetical characters in your program’s output. For instance, if k is 27,

A should not become [ even though [ is 27 positions away from A in ASCII, per http://www.asciichart.com/[asciichart.com]; A should become B, since B is 27 positions away from A, provided you wrap around from Z to A.

Your program must output plaintext: (without a newline) and then prompt the user for a string of plaintext (using get_string).

Your program must output ciphertext: (without a newline) followed by the plaintext’s corresponding ciphertext, with each alphabetical character in the plaintext “rotated” by k positions; non-alphabetical characters should be outputted unchanged.

Your program must preserve case: capitalized letters, though rotated, must remain capitalized letters; lowercase letters, though rotated, must remain lowercase letters.

After outputting ciphertext, you should print a newline. Your program should then exit by returning 0 from main.

My code:

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

int main(int argc, string argv[])
{
    //check if k inputed
    if (argc != 2)
    {
        printf("Usage: ./caesar key\n");
        return 1;
    }
    //value k is the number after ./caesar
    int k = atoi(argv[1]) % 26;
    int x = 0;
    int s = strlen(argv[1]);
    //check if k is a positive integer
    if (k < 0)
    {
        printf("Usage: .caesar key\n");
        return 1;
    }
    else
    {
        //check for arguments
        for (int i = 0; i < s; i++)
        {
            if (isalpha (argv[1][i]))
            {
                continue;
            }
            else if (isalnum (argv[1][i]))
            {
                x++;
            }
            else
            {
                continue;
            }
        }
        if (x != s)
        {
            printf("Usage: ./caesar key\n");
        }
        else if (x == s)
        {
            //get plaintext
            string plain_text = get_string("plaintext: ");
            printf("ciphertext: ");
            for (int y = 0; y <= strlen(plain_text); y++)
            {
                //change letters
                if (isalpha(plain_text[y]))
                {
                    char p = plain_text[y];
                    int cipher_int = p + k;
                    if (isupper(p))
                    {
                        while(cipher_int >= 90)
                        {
                            cipher_int -= 26;
                        }
                            char cipher_text = cipher_int;
                            printf("%c", cipher_text);
                    }
                    if (islower(p))
                    {
                        while(cipher_int >= 122)
                        {
                            cipher_int -= 26;
                        }
                            char cipher_text = cipher_int;
                            printf("%c", cipher_text);
                    }
                }
                else
                {
                    printf("%c", plain_text[y]);
                }
            }
            printf("\n");
        }
    }
    return 0;
}

Answer 1

Your check for digits is very cumbersome and does not cause the program to return 1 as required if the argument is incorrect.

Here is a simpler test:

        //check for arguments
        for (int i = 0; i < s; i++) {
            if (!isdigit((unsigned char)argv[1][i])) {
                printf("Usage: ./caesar key\n");
                return 1;
            }
        }

Also note that you should stop the encoding loop when the index == the length of the string. therefore the operator should be <.

Another problem is the use of isalpha() and similar functions from <ctype.h> with char values. These functions are undefined for negative values (except EOF). Some platforms define char as signed by default, making isalpha(plaintext[y]) have undefined behavior if the user typed non ASCII text. Cast the argument as (unsigned char) to avoid this problem.

Furthermore, you should not use hardcoded ASCII values such as 90 and 122, use character constants such as 'a' and 'z' for better readability. Doing so would make another error in your encoding loop more obvious: while(cipher_int >= 90) should be if (cipher_int > 'A') and while(cipher_int >= 122) should be if(cipher_int > 'z').

Here is a modified version:

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

int main(int argc, string argv[])
{
    // check for a single command line argument
    if (argc != 2) {
        printf("Usage: ./caesar key\n");
        return 1;
    }
    char *arg = argv[1];
    if (*arg == '\0') {
        printf("caesar: key cannot be an empty string\n");
        return 1;
    }
    // check that the argument is a non negative number
    for (size_t i = 0; arg[i]; i++) {
        if (!isdigit((unsigned char)arg[i])) {
            printf("Usage: ./caesar key\n");
            return 1;
        }
    }
    // value k is the shift number after ./caesar
    int k = atoi(argv[1]) % 26;

    // get plaintext
    string plain_text = get_string("plaintext: ");
    printf("ciphertext: ");
    for (size_t i = 0; plain_text[i] != '\0'; i++) {
        unsigned char c = plain_text[i];
        // change letters
        if (islower(c)) {
            putchar('a' + ((c - 'a') + k) % 26);
        } else
        if (isupper(c)) {
            putchar('A' + ((c - 'A') + k) % 26);
        } else {
            putchar(c);
        }
    }
    printf("\n");
    return 0;
}

Answer 2

It appears that your wrapping is not working correctly. I found that when I used 3 as the key and put "The quick fox jumps over the lazy brown dog." as the plain text, "brown" became "eur`q" when it should be "eurzq". I think you're using >= in your wrapping comparisons when you should use >.