CS50x Caesar - Segmentation fault when inserting isalpha(argv[1])

Question

For the CS50x Problem Caesar I have created a program that encrypts messages using Caesar’s cipher.

For this, the program must recognize that a command-line argument is only a number. So no two or more numbers, no number below zero, and no text.

But as soon as I add the check if it is a text with || isalpha(argv[1]), the program does not work anymore.

The terminal prints the following when I try to run the program:

Segmentation fault

Can anyone tell me what is the problem with the code

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

int main(int argc, string argv[])
{
    int kkey = 0;

    // Check if correct command-line arguments
    if (argc != 2 || atoi(argv[1]) < 0 || isalpha(argv[1])) //segfault here
    {
        printf("./caesar key\n");
        return 1;
    }
    else
    {
        kkey = atoi(argv[1]);
    }

    // Ask for Plaintext to encrypt
    string plaintext = get_string("plaintext: ");

    for (int i = 0, n = strlen(plaintext); i < n; i++)
    {
        if (isalpha(plaintext[i]) && islower(plaintext[i]))
        {
            plaintext[i] = (plaintext[i] - 'a' + kkey) % 26 + 97;
        }
        else if (isalpha(plaintext[i]) && isupper(plaintext[i]))
        {
            plaintext[i] = (plaintext[i] - 'A' + kkey) % 26 + 65;
        }

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

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

Thank you very much for your help.

Answer 1

As said by @Gerhardh, you can't use strings as argument of isalpha, you need a loop to check each character of the string.

In any case that is not the best approach, using a negated isdigit would be a better option, because it accounts for all the other non numeric characters.

//...
// Check if correct command-line arguments
if (argc != 2 || atoi(argv[1]) < 0) 
{
    printf("./caesar key\n");
    return 1;
}

for(size_t i = 0; i < strlen(argv[1]); i++){
    if(!isdigit(argv[1][i])){ //if one of the characters is not a digit 0-9
        puts("./caesar key\n");
        return 1;
    }
}

kkey = atoi(argv[1]); //no else needed
//...

---

Note that atoi will invoke undefined behavior if the converted value is not representable by an int.

You can use strtol for a more robust alternative.

The link is for Linux man page which I find quite nice, but this is cross-platform.

---

Again, as stated by @Gerhardh, using character codes may backfire, in this case you are using ASCII encoding, but there are others, this makes your code less portable, use the character instead, 26 + 'a' and 26 + 'A'.