Understanding "Pset 2: Caesar" code in CS50

Question

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

int main(int argc, string argv[])
{
    if (argc == 2)
    {
        string cmd = argv[1];

        for (int i = 0, n = strlen(cmd); i<n ; i++)
        {
            if (isdigit(cmd[i]))
            {
                continue;
            }
            else
            {
                printf("Usage: %s key \n", argv[0]);
            }
        }
    }
    else
    {
        printf("Usage: %s key \n", argv[0]);
    }
}

I am following the CS50 course, and I have gotten to the "Pset2: Caesar" problem. I am able to take the command line argument and check them if they are digits or not.

The problem I am facing, is that after checking the validity of the arguments, I want the program to return that value. I want to check if the given input is numerical only. If it is, I want the program to use that numerical argument value for computing the cipher. The problem is_ I am using a loop to check the input, and I have to use continue; I am confused, since if I remove continue; the program will fail the task of validating the key.

Example: If I replace continue; with printf("True\n");, the program prints TrueTrue after giving 20 as input. After giving 20x as input, I get TrueTrueUsage: ./Caesar key whereas ideally, it should only return Usage: ./Caesar key.

The problem statement link: https://cs50.harvard.edu/x/2020/psets/2/caesar/

Answer 1

You are complicating the logic of confirming that the first argument to the program is all digits. You really have three tasks:

1. validate that at least one argument is given;
2. loop over the argument to validate each character is a digit;

• if non-digit found, output error, show usage, return EXIT_FAILURE (1)

3. valid argument -- output key.

While it is perfectly fine to check (argc == 2) to require exactly one argument on the command line, it is often helpful to think about what is needed, at minimum. There you need at least one argument, so a check of (argc < 2) identifies the lack of an argument, but does not prevent entering more than 1 argument if you take additional optional arguments. For example:

    if (argc < 2 ) {    /* validate 1 argument given */
        fprintf (stderr, "error: insufficient input,\n"
                        "usage: %s key (all digits)\n", argv[0]);
        return 1;
    }

(note: on error, there is no need for an else statement. Simply handle the error and exit. That saves a level of indention throughout the rest of your code.)

Validating that each character is a digit requires simply looping over each character and checking with isdigit(). You do not need strlen(). In C a string is terminated by the nul-terminating character '\0' (which has the ASCII value of 0). To loop over a string, all you need is:

    string cmd = argv[1];           /* assing to cmd */
    
    for (int i = 0; cmd[i]; i++)    /* loop over each char in cmd */
        if (isdigit((unsigned char)cmd[i]) == 0) {  /* if non-digit, handle error */
            fprintf (stderr, "error: '%c' is non-digit.\n"
                            "usage: %s key (all digits)\n", cmd[i], argv[0]);
            return 1;
        }

(note: again, when an error condition requiring exit is reached, there is no need for an else, simply handle the error and exit)

Also note the value passed to the isdigit() macro (as for all ctype.h macros) must have the value of an unsigned char or EOF so the cast to unsigned char is necessary. See man 3 isalpha (understand that the value of a char will always be within unsigned char, but build good habits early)

After validating that all characters are digits, all that remains is outputting your key. Putting it altogether you would have:

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

int main (int argc, char **argv) {
    
    if (argc < 2 ) {    /* validate 1 argument given */
        fprintf (stderr, "error: insufficient input,\n"
                        "usage: %s key (all digits)\n", argv[0]);
        return 1;
    }
    
    string cmd = argv[1];           /* assing to cmd */
    
    for (int i = 0; cmd[i]; i++)    /* loop over each char in cmd */
        if (isdigit((unsigned char)cmd[i]) == 0) {  /* if non-digit, handle error */
            fprintf (stderr, "error: '%c' is non-digit.\n"
                            "usage: %s key (all digits)\n", cmd[i], argv[0]);
            return 1;
        }
    
    printf ("key: %s\n", cmd);      /* output good key */
}

Example Use/Output

No argument:

$./bin/pset2_ceaser
error: insufficient input,
usage: ./bin/pset2_ceaser key (all digits)

Invalid argument:

$ ./bin/pset2_ceaser 123foo
error: 'f' is non-digit.
usage: ./bin/pset2_ceaser key (all digits)

Good input:

$ ./bin/pset2_ceaser 12345
key: 12345

Look things over and let me know if you have further questions, or if I misread your question in any way.

Answer 2

You can break the loop early if the code hits a character that is not a digit:

   if ( !isdigit( cmd[i] ) )
   {
       printf("Usage: %s key \n", argv[0]);
       break;
   }

There's also the function strtol() which converts the argument to an integer. It will handle the errors for you.

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

int main( int argc, char *argv[] )
{
    if (argc == 2)
    {
        char *error_at = NULL;

        long rotation = strtol( argv[1], &error_at, 10 );

        if ( rotation == 0 )
        { 
            if ( error_at != argv[1] )
                fprintf( stderr, "A rotation of zero is ineffective\n" );

            else if ( error_at == argv[1] )
                fprintf( stderr, "That's not a number!\n" );

            exit( 1 );
        }
        else
        {
            // Cypher is limited to A-Z
            rotation %= 26;

            printf( "DEBUG: rotation is %ld\n", rotation );
        }
    }
    else
    {
        printf ("Usage: %s key \n", argv[0]);
        exit( 1 );
    }

    return 0;
}