Caesar prints out the desired output but fails the check50

Question

I managed to write the code for caesar and it presents the desired values as output.

However, when I execute check50, apart from a smiley on all other points, this is what I get:

:( handles non-numeric key timed out while waiting for the program to exit

I cannot understand what it means and cannot figure out how to solve it.

I would be very obliged if you could help me out.

Thank you so much!

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

int main(int argc, string argv [])
{
  if (argc == 2 && isdigit(*argv[1]))
   {
      string t = argv [1];
      int k = atoi ( t );
      string s = get_string ("plaintext: ");
      printf ("ciphertext:" );
      for (int i = 0, n = strlen(s); i < n; i++)
      {
         char c = s[i];
         if (isalpha (c))
         {
            if (isupper(c))
            {
               int x = ((int) c - 65 + k) % 26 + 65;
               printf ("%c", (char) x);
            }
            else
            {
               int x = ((int) c - 97 + k) % 26 + 97;
               printf ("%c", (char) x);
            }
         }
         else
         {
            printf ("%c", c);
         }
      }

      printf ("\n");
      return 0;
   }
   else
   {
       printf("Usage: ./caesar key \n");
       return 1;
   };
} 

Answer 1

It fails because the test for non-numeric key is bad. It won't catch a key where the first char is a number and any other char is a non-numeric.

Look at this code:

       if (argc == 2 && isdigit(*argv[1]))

This will ONLY look at the first char in argv[1]. The isdigit()function only looks at single chars, not at entire strings. *argv[1] is the address of the first char in the string argv[1]. So, it's only looking at the first char, not every char.

The code needs a for loop to step through the string and check each char, one at a time.

The reason that it times out is that the test passes a key such as "1s" and waits for user input. Since check50 isn't giving it any input, the test times out.

Then, of course, there's the bad practice of surrounding almost the entire program code in an if statement's code block and putting the else clause at the bottom. (I've discussed this frequently in other threads.)

The best practice is to compartmentalize code into small blocks of code that only does one small task and then move on to the next.

In this case, check the number of parameters, aka, argc and if it's bad, print an error message and exit. THEN, validate the input parameters. If they're bad, print an error message and exit. THEN, continue with the rest of the code, but don't have anything connecting the input validation to the remaining code. Each of these sections of code should be able to stand as independent code that works without any of the other code.

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