Is redundant condition checking against best practices?

Question

I have been developing software for the past three years, but I just recently awoke to how ignorant I am of good practices. This has led me to begin reading the book Clean Code, which is turning my life upside for the better, but I am struggling to get insight into some of the best approaches for writing my programs.

I have a Python program in which I...

1. use argparse required=True to enforce two arguments, which are both file names. the first is the input file name, the second is the output file name
2. have a function readFromInputFile which first checks to see that an input file name was entered
3. have a function writeToOutputFile which first checks to see that an output file name was entered

My program is small enough that I am lead to believe that the checking in #2 and #3 is redundant and should be removed, thus freeing both functions from an unnecessary if condition. However, I have also been led to believe that "double-checking is ok" and may be the right solution in a program where the functions could be called from a different location where the parsing of arguments does not occur.

(Also, if the read or write fails, I have a try except in each function to raise an appropriate error message.)

My question is: is it best to avoid all redundant condition checking? Should the logic of a program be so solid that checks need only be made once? Are there any good examples that illustrate this or the converse?

EDIT: Thank you all for the answers! I have learned something from each. Seeing so many perspectives gives me a much better understanding of how to approach this problem and determine a solution based on my requirements. Thank you!

Answer 1

What you are asking for is called "robustness", and there is no right or wrong answer. It depends on the size and complexity of the program, the number of people working in it, and the importance of detecting failures.

In small programs you write alone and only for yourself, robustness is typically a much smaller concern than when you are going to write a complex program which consists of multiple components, maybe written by a team. In such systems, there are boundaries between the components in form of public APIs, and at each boundary, it is often a good idea to validate the input parameters, even if "the logic of the program should be so solid that those checks are redundant". That makes the detection of bugs quite easier and helps to keep the debugging times smaller.

In your case, you have to decide for yourself, which kind of life cycle you expect for your program. Is it a program you expect to be used and maintained over years? Then adding a redundant check is probably better, since it will not be unlikely that your code will be refactored in the future and and your read and write functions might be used in a different context.

Or is it a small program just for learning or fun purposes? Then those double checks won't be necessary.

In the context of "Clean Code", one could ask if a double check violates the DRY principle. Actually, sometimes it does, at least to some minor degree: input validation can be interpreted as part of the business logic of a program, and having this at two places might lead to the usual maintenance problems caused by the violation of DRY. Robustness vs. DRY is often a tradeoff - robustness requires redundancy in code, whilst DRY tries to minimize redundancy. And with increasing program complexity, robustness becomes more and more important than being DRY in validation.

Finally, let me give an example what that means in your case. Lets assume your requirements change to something like

• the program shall also work with one argument, the input file name, if there is no output file name given, it is automatically constructed from the input file name by replacing the suffix.

Does that make it likely you need to change your double validation in two places? Probably not, such a requirement leads to one change when calling argparse, but no change in writeToOutputFile: that function will still require a filename. So in your case, I would vote for doing the input validation twice, the risk of getting maintenance problems because of having two places to change is IMHO much lower than the risk of getting maintenance problems because of masked errors caused by too few checks.

Answer 2

Redundancy isn't the sin. Needless redundancy is.

1. If readFromInputFile() and writeToOutputFile() are a public functions (and by Python naming conventions they are since their names didn't start with two underscores) then the functions might someday be used by someone who avoided argparse altogether. That means when they leave off the arguments they don't get to see your custom argparse error message.

2. If readFromInputFile() and writeToOutputFile() check for parameters themselves, you again get to show a custom error message that explains the need for filenames.

3. If readFromInputFile() and writeToOutputFile() don't check for parameters themselves, no custom error message is shown. User will have to figure out the resulting exception on their own.

It all comes down to 3. Write some code that actually uses these functions avoiding argparse and produce the error message. Imagine you haven't looked inside these functions at all and are just trusting their names to provide enough understanding to use. When that's all you know is there any way to be confused by the exception? Is there a need for a customized error message?

Turning off the part of your brain that remembers the insides of those functions is hard. So much so that some recommend writing the using-code before the code that gets used. That way you come to the problem already knowing what things look like from the outside. You don't have to do TDD to do that but if you do TDD you'll already be coming in from outside first.

Answer 3

The extent to which you make your methods stand-alone and re-usable is a good thing. That means methods should be forgiving in what they accept and they should have well-defined outputs (precise in what they return). That also means that they should be able to gracefully handle everything passed to them and not make any assumptions about the nature of the input, quality, timing etc.

If a programmer is in the habit of writing methods that make assumptions about what's passed in, based on ideas like "if this is broken, we have bigger things to worry about" or "parameter X can't have value Y because the rest of the code prevents it", then all of a sudden you don't really have independent, decoupled components any more. Your components are essentially dependent on the wider system. That is a kind of subtle tight coupling and leads to exponentially increasing total cost of ownership as system complexity increases.

Note that this may mean that you're validating the same information more than once. But this is OK. Each component is responsible for it's own validation in it's own way. This isn't a violation of DRY, because the validations are by de-coupled independent components, and a change to the validation in one doesn't necessarily have to be replicated exactly in the other. There is no redundancy here. X has a responsibility to check it's inputs for its own needs and pass some to Y. Y has it's own responsibility to check it's own inputs for its needs.

Answer 4

Assume you have a function (in C)

void readInputFile (const char* path);

And you can't find any documentation about the path. And then you look at the implementation and it says

void readInputFile (const char* path)
{
    assert (path != NULL && strlen (path) > 0);

Not only does this test the input to the function, but it also tells the user of the function that the path is not allowed to be NULL or an empty string.

Answer 5

The check is redundant. Fixing this though, requires that you remove readFromInputFile and writeToOutputFile and replace them with readFromStream and writeToStream.

At the point where the code receives the file stream, you know you have a valid stream connected to a valid file or whatever else a stream can be connected to. This avoids redundant checks.

You might then ask, well, you still need to open the stream somewhere. Yes, but that happens internally in the argument parsing method. You have two checks there, one to check that a filename is required, the other is a check that the file pointed by the filename is valid file in the given context (e.g. input file exists, output directory is writable). Those are different type of checks, so they aren't redundant and they happen within the argument parsing method (application perimeter) rather than within the core application.

Answer 6

In general, double-checking isn't always good or bad. There are always many aspects of the question in your particular case on which the matter depends. In your case:

• How big is the program? The smaller it is, the more obvious it is that the caller does the right thing. When your program grows larger, it becomes more important to specify exactly what the preconditions and postconditions of each routine are.
• the arguments are already checked by the argparse module. It's often a bad idea to use a library and then do its job yourself. Why use the library then?
• How likely is it that your method will be reused in a context where the caller doesn't check arguments? The more likely it is, the more important it is to validate arguments.
• What happens if an argument does go missing? Not finding an input file will probably stop processing outright. That's probably an obvious failure mode that's easy to rectify. The insidious kind of errors are those where the program merrily keeps working and produces wrong results without you noticing.

Answer 7

Your double-checks seem to be at places where they are used rarely. So these checks are simply making your program more robust:

A check too much won't hurt, one too less might.

However, if you are checking inside a loop that is repeated often, you should think about removing redundancy, even if the check itself is in most times not costly compared to what follows after the check.

Answer 8

Perhaps you could change your point of view:

If something goes wrong, what is the outcome? Will it do harm to your application/the user?

Of course you could always argue, whether more or less checks are better or worse but that is a rather scholastic question. And since you are dealing with real world software, there are real world consequences.

From the context you are giving:

• one input file A
• one output file B

I assume you are doing transformation from A to B. If A and B are small and the transformation is small, what are the consequences?

1) You forgot to specify where to read from: Then the outcome is nothing. And the execution time will be shorter than expected. You look at the result - or better: look for a missing result, see that you invoked the command in a wrong way, start over and all's fine again

2) You forgot to specify the outputfile. This results in different scenarios:

a) The input is read at once. Than the transformation starts and the result should be written, but instead you receive an error. Depending on the time, your user has to wait (dependend on the mass of data which schould be processed) this could be annoying.

b) The input is read step by step. Then the writing process immediately quits like in (1) and the user starts over again.

Sloppy checking could be seen as OK under some circumstances. It wholly depends on your usecase and what your intention is.

Additionally: You should avoid paranoia and to not do too many doublechecks.

Answer 9

I would argue that the tests are not redundant.

• You have two public functions that require a filename as an input parameter. It is appropriate to validate their parameters. The functions could potentially be used in any program that needs their functionality.
• You have a program that requires two arguments that must be filenames. It happens to use the functions. It is appropriate for the program to check its parameters.

While the filenames are being check twice, they are being checked for different purposes. In a small program where you can trust the parameters to the functions have been verified, the checks in the functions could be considered redundant.

A more robust solution would have one or two filename validators.

• For an input file, you may want to verify that the parameter specified a readable file.
• For an output file, you may want to verify that parameter is a writable file or a valid filename that can be created and written to.

I use two rules for when to perform actions:

• Do them as early as possible. This works well for things that will always be required. From this program's point of view, this is the check on the argv values, and subsequent validations in the programs logic would be redundant. If the functions are moved to a library, then they are no longer redundant, as the library cannot trust that all callers have validated the parameters.
• Do them as late as possible. This works extremely well for things that will rarely be required. From this program's point of view, this is the checks on the function parameters.