pset 2 vigenere - handles lack of argv[1]

Question

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;
    }
}

Answer 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. ;-)