Parsing argv with while-switch construct

Question

I've been thinking about how to create a robust function that parses the command line arguments for valid filenames. I came up with a while-switch construct because that allows me to reduce redundancy (no break-statement at the end of case 2).

As I am pretty new to programming, I can't really tell the quality of this code. Can you think of a way to improve this code in robustness as well as in elegance?

void spellchecker::openFiles(int argc, char* argv[]) {
    bool filesOpen = false;
    std::string dict_filename;
    std::string text_filename;

    while (filesOpen == false) {
        switch (argc) {

            case 2:
                printf("You already entered the name of the dictionary file as \"%s\". Is this correct? (y/n): ", argv[1]);
                if (std::cin.get() == 'y') {
                    printf("Ok, please enter the name of the text file you want to check: ");
                    std::cin >> text_filename;
                    break;
                }
                /* no break so user can enter both filenames*/
            default:
            case 1:
                printf("Please enter the name of the dictionary: ");
                std::cin >> dict_filename;
                printf("Please enter the name of the text file to check: ");
                std::cin >> text_filename;
                break;

                case 3:
                    dict_filename = argv[1];
                    text_filename = argv[2];
                    break;
        } // switch

                if (!dict_file.is_open()) {
                    dict_file.open(dict_filename.c_str(), std::ifstream::in);
                }
                if (!text_file.is_open()) {
                    text_file.open(text_filename.c_str(), std::ifstream::in);
                }
                if (dict_file.good() && text_file.good()) {
                    filesOpen = true;
                }
    } // while
}

Answer 1

This solution does not sit well with me. It looks like you're doing too much: checking how to take the input, taking input, and then checking whether that input worked all at once.

Before looking at the code, though, it's worth asking whether this is really the interface that you want to provide to the user. The way you've designed it, it looks like it is meant to be run by a human, as the requests for input will only muddle the output in a script. Furthermore, spell-checkers are usually non-destructive: running it with the wrong input file or the wrong dictionary file shouldn't lose you any data.

All in all, this makes double-checking with the user whether the dictionary file is correct questionable. It is also inconsistent: you do validate the input in that case, but don't in the case that both files are provided, so the user has to remember that if he intends to rely on the input being checked.

My first suggestion is thus to change your interface: only prompt for things that are not already provided, and if you detect an error, report it and exit.

Now to the code: you have a few cases where the program can go in an infinite loop. This happens if the user closes std::cin, or if the user provided two filenames via argv and then at least one failed to open: you'll keep trying to open it and filesOpen will never be true.

Secondly, you're using operator>> in most places, but use std::cin.get() to ask the user whether he's sure. This will likely break if the user provides one command-line argument and then some file fails to open.

Thirdly, the condition of your loop seems strange to me. Why use an extra bool that you then set at the very end instead of using break or return to get out of the loop, or, better, checking the condition directly? You can use a do-while loop if you want to save yourself one check, though I wouldn't bother as the performance effect will be negligible, while the code will be clearer.

As for the switch construct: I'd use a helper function or two instead. The code is sufficiently small for it to be clear, especially with the comment, but I don't think it wins you anything over two function calls. In general, the function looks too big to me, but I've already covered that above.

Last and least, mixing std::printf and std::cin is strange. I can't come up with any technical problems, but mixing two styles without a good reason is generally more trouble than it's worth.

Answer 2

You might want to change the interface of openFiles to this:

void spellchecker::openFiles(int argc, char** argv)

The following is a quote from James Kanze:

I never declare a function with an array argument; I always use the pointer (the rare times I need it).

On the other hand, many textbooks show main() as

int main( int argc, char* argv[] )

rather than

int main( int argc, char** argv )

(which is the way I write it).

That's even how it is presented in the standard. And that influences people.