Is it a bad practice to use conditionals with functions that change program's state?

Question

The title might be a bit vague, so let me explain. Let's assume we have a function that does something (changes state of the program), for example a function that creates a file. That function returns True if the file is created and False if the file isn't created.

Now, we want to use that function in a conditional, for example:

if (createFile() == false)
    // log: we cannot create file

And we can also do it the following way:

boolean fileCreated = createFile()
if (fileCreated == false)
    // log: we cannot create file

The question is if the first case is worse than the second in terms of readability and clarity and which one is recommended to be used?

My reasoning is that because someone reading the code might not be familiar with the function's internals so in the first case he might assume that the function createFile() doesn't alter the state (since often these functions are predicate functions)?

Answer 1

It's a bit of a judgement call but I would say that if you need to call createFile() early in a method it's slightly better to store the result in a variable. This is because if someone updates the code later and needs to check the fileCreated state, it's common to copy the condition from the existing block of code. If you use the method directly, the editor needs to introduce the variable and update the existing condition. Not really the end of the world so I wouldn't get hung up on it. If the last statement was return createFile(), I wouldn't introduce a variable.

Really, though it's preferable to not have to check the same condition more than once in a method so avoiding that entirely is optimal. That would render the above pointless. So it's not really a cut-and-dried thing. It kind of depends on what the skill level of the team is.

I related but different case is that, even if the method does not modify anything but can return different results at different calls, it's often necessary to capture the result locally for correctness.

Answer 2

To me, the main code smell is that a createFile() method returns a boolean indicating success. That reminds me of programming styles from the 1970s.

A method named createFile() implies a contract that it creates the file, and if it can't, the contract isn't fulfilled, and the method should throw an exception. With the boolean-returning version, a slightly careless user, accustomed to current programming practices, will see that name and call the method, ignoring the boolean return value, and assume that the file has been created.

If you want the contract to allow for not creating a file under some circumstances, you should indicate that in the name, e.g. like createFileIfPossible().

Typically, user code creates a file because it's needed for proceeding, so the default exception handling is what you want, aborting all further computation up to a place where you know how to continue. Place a catch there, and log the exception there. Then there will be no more need for the code fragment from your question.

Answer 3

Using the return-value from a function directly, instead of saving it in a variable, is perfectly fine, if you only need it the once. In fact, not introducing superfluous artifacts is a good idea, as thus you cannot loose track of them, nor name them badly.

What is not fine is that functions name. I wouldn't expect it to return a bool, but a File or whatever, and throw an exception on failure. In languages supporting that it should probably be a ctor.
Alternatively, renaming it tryOpenFile() seems acceptable.
Another oddity is that there is no argument for the file-name.

Answer 4

The code needs to be easy readable. If you make the developer trying to find what's really happening in your code, there is something wrong.

Both cases are readable for me, but this not means that they cannot be improved. I understand that the return of "create file" is not so obvious in terms of meaning. My idea is create a new method (instead of a variable) to make the code reading more natural.

So, you can improve a little with:

if (!isFileCreated(createFile())
    // log: we cannot create file

Answer 5

Ignore the actual calls for a second, because they are an implementation detail and should be handled and such. Your first task is: Either create a file and set a boolean "fileCreated" to true, or try to create a file but fail, clean up everything as if you had never attempted to create a file, and set a boolean "fileCreated" to false.

Your second task is: If the file was not created, log the fact.

Now write the code for the second task. You should be able to do this without having written the code for the first task. As a result, you can't have a condition like "if (createFile() == false). Your first code example comingled the file creation and the test. It was only a small bit of code, so you get away with it, but it will get ugly in bigger cases.