0

I have this at the very top of my program to check if the user input is correct, but somehow I'm having segmentation fault? Could someone tell me what went wrong with my code? Thanks!

// Check correctness of command line argument
for (int i = 0; i < strlen(argv[1]); i++)
{
    if (!isalpha(argv[1][i]) || argc != 2)
    {
        printf("Error - Incorrect command line argument.\n");
        return 1;
    }
}


enter image description here

1

It's all about what is done first! Look at the line:

    if (!isalpha(argv[1][i]) || argc != 2)

This will first test for isalpha and then will test for the value of argc. Now, say argc = 1, which means that there are no parameters in the command. (The program call, ./vigenere, is argv[0].) Since argv[1] doesn't exist, the first test, the call to isalpha, throws an error. (If there were 3 parameters, all alpha, the first test would pass but the second would fail, and no runtime errors would happen.)

On the other hand, if the code were to test the value of argc first, it guarantees that argv[1] exists before the test is executed.

Remember, when there are multiple test conditions in an if statement, they are executed in order. As soon as one fails, the whole test has failed. No further tests will be checked or executed because they don't matter. It's also because, like this example, the later tests may depend on passing earlier tests so that they don't throw an error. ;-)

Think of it this way, it's like checking if the ball in the bag is blue before you check if there's a ball at all. The following should work:

    if (argc != 2  ||  !isalpha(argv[1][i]) )

For efficiency of execution, it would be better to check argc before the for loop. Including it here, inside the loop means that argc is checked on every pass through the loop. However, you also have to decide if it's worth changing. If argv[1] is only a few dozen chars or so, it probably doesn't make much difference. If this were a different program, however, and the loop were to execute thousands or millions of times, it would make a big difference.

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

6
  • I tried checking the length of argc before the proceeding to the for loop to check if each character is an alphabet, but it seems to give me random errors like 'use of undeclared identifier' after I try to compile the program?
    – Michelle
    Jun 20 '18 at 5:40
  • Exactly how did you do that?
    – Cliff B
    Jun 20 '18 at 5:53
  • I checked if argc != 2 then print the error and return 0, then proceed with 'for (int i = 0; i < strlen(argv[1]); i++)' and if !isalpha(argv[1][i]) to check if the input is all alphabet. I tried reversing both statements as you suggested but it still failed the test
    – Michelle
    Jun 20 '18 at 6:20
  • Well, if you return 0 instead of 1, it will fail. When a program returns 0, it's telling the operating system that it completed without error. Try returning something other than 0.
    – Cliff B
    Jun 20 '18 at 6:27
  • My bad, that was a typo! I returned 1 for both if statements.
    – Michelle
    Jun 20 '18 at 6:30

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .