I believe my code is right but it's not giving output and pops segmentation fault

Question

This is my code for Caesar Pset2. The code must be doing right but it's broken somehow.

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

int main(int argc, string argv[])
{
    char c[strlen(argv[1])];
    for (int i=0; i<strlen(argv[1]); i++ )
    {
        c[i] = argv[1][i];
    }

    int k =0;

    printf("%s\n", c);
    if (argc ==2 && isdigit(c[0]))
    {
        k = c[0];
        string ip = get_string("plaintext:");    

        for (int i=0; i<strlen(ip); i++ )
        {
            if (islower(ip[i]))
            {
                printf("%c", ((((ip[i] + k)- 97) % 26) - 97));
            }
            else if (isupper(ip[i]))
            {
                printf("%c", ((((ip[i] + k)- 65) % 26) - 65));
            }
        }
    }
    else
    {
        printf("Usage: ./caesar key");
    }
}

This is the error Segmentation fault when the final else loop is executed i.e. it shows Segmentation fault when command line argument is just

~/caesar/ $ ./c

but does move forward if command line argument is

~/caesar/ $ ./c 5

Answer 1

If code generates a seg fault, something is definitely wrong, period! End of story!

Common newbie error. The code is trying to do something with argv[1] before verifying that argc == 2, and by extension, before verifying that argv[1] exists.

Simply put, if there's no key, then argc = 1. That means that argv[1] doesn't exist. So, when the code tries to use it in the first line of the program, it generates a seg fault.

ANY code MUST check argc before doing anything with argv[].

Programming tips: This code uses the following line to test input validity:

if (argc ==2 && isdigit(c[0]))

Most of the code is then enclosed in the curly braces that follow, with an else clause at the end. This is a bad practice and another common practice by new programmers, and should be avoided. Imagine if this were production code in the real world and you had 1000 lines of code instead of just a few. Now, imagine that you have to come back to it in 6 months, or someone who's never seen it before, to make changes. It would be very easy to forget or not realize that the else clause way down at the end is connected to an if statement at the very beginning.

The best practice is this. Validate the input in a small number of lines. If the data is invalid, abort the program. The else clause is then unnecessary. Here's the basic pseudocode for this concept.

int main(int argc, string argv[])
{
    if( argc != 2)  // or whatever number it should be
    {
        printf(error message);
        return 1;   // 1, or another number, indicating a programmer created error.
    }

    if( some other check is wrong)    // meaning other input is bad
    {
        // do similar error messaging and return.
    }


    //   begin the major part of the program, 
    //   with no orphaned else clause at the end
}

By doing this, all the input parameter checks are done at the beginning and contained in a handful of lines, with no linkage or dependance on code elsewhere in the program.

Next tip - more of a hint. The atoi() function is your friend. ;-)

Now, a little tale of an illness suffered by all programmers at one time or another. There's a disease called the "It cannot be I" syndrome. The short version is that programmers convince themselves that they've written perfect code and there must be something wrong elsewhere, simply because they can't find the error. In 1 case out of a million or so, they turn out to be correct. But the other 999,999 times, there's a flaw in the code. It can be syntax, a misuse of a function, or it can be a deep logic error. But, they never figure it out because they've convinced themselves that they have written perfect code.

The moral of the story is that when a programmer discovers that there's a problem, they MUST accept that a problem exists! There's no use denying it. Then, they can move on to the next 3 steps in order - locate, identify, and fix.

Don't be the programmer that says "It cannot be I!!!" ;-) Embrace the problem. "Where is the error!!!"

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