4

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)?

  • 5
    I guess readability is in the eye of the reader as the only thing that stands out for me is that horrible == false, rather than using !. Aside from that, the first version looks better to me, but that's really just pure opinion. – David Arno Mar 26 '18 at 14:04
  • 2
    I don't see why the second version conveys the idea that createFile() will change state, but the first version doesn't. – David Arno Mar 26 '18 at 14:07
  • 2
    Typically if a function is supposed to change the state but fails to do so it raises an exception. Then your createFile() would simply be void and the 'if' would not be needed at all. – yegodm Mar 26 '18 at 14:11
  • 1
    @yegodm, if that truly is typical, then folk typically write bad code. Throw exceptions in exception circumstances, not just because the golden path through the code didn't occur. – David Arno Mar 26 '18 at 14:13
  • 3
    I don't think there is a problem with this. Just don't name methods with side effects as "isFileReady()" which suggests immutability. – Neil Mar 26 '18 at 14:13
4

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.

  • 1
    "Not really the end of the world so I wouldn't get hung up on it" I don't like making more-verbose-than-necessary code now, just to optimize for the off chance i'll maybe need to change it in the future. The change is just IDE refactoring, no point trying to "avoid" the need for that, it's trivial. – Alexander Mar 26 '18 at 16:02
  • @Alexander Like I said, it's not really cut-and-dried. The problem is that you need know to refactor. The IDE is not going to tell you that you need to do that instead of copy-and-pasting the code. At the end of the day, it's the person that copied and pasted who would be at fault. I've seen this kind of mistake cause bugs though. Ideally it would be caught in testing. I wouldn't fault anyone for not wanting the extra line and I tend to do it your way. Maybe if calling it more than once was really bad, I might add this as a caution. – JimmyJames Mar 26 '18 at 16:12
  • 2
    Yeah I agree, you have to be mindful of sideeffects when copying function calls. Although I would have approached this differently, instead making createFile return Optional<File>, assigning it if necessary, otherwise just doing if (createFile().isPresent()). Theres a bit more going on there, which I think would make people take a second to reconsider copy/paste, in a similar way to how the uncertainty around signless intersections makes people drive more cautiously. – Alexander Mar 26 '18 at 16:15
  • 1
    You have to think about idioms as well. Many common stream reading patterns in C/C++ modify state in the condition of a while loop. – Frank Hileman Mar 26 '18 at 18:16
  • @FrankHileman I think that point is what pushed me to this answer. If you call something in a way that looks like such an idiom, there's a better chance that someone will use it in that way. Assigning the result to a (final) variable is an idiom that suggest that this is something that should be done once and only once. – JimmyJames Mar 26 '18 at 18:21
10

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.

  • 1
    +1 for throwing an exception if the create fails. History tells us that programmers are notoriously lax about checking return codes, which allows the error to propagate, possibly in interesting ways, and can make tracking it back difficult. Exceptions usually provide immediate traceback. – John R. Strohm Mar 26 '18 at 20:31
  • 5
    Better tryCreateFile(). It's shorter and no less descriptive. – Deduplicator Mar 26 '18 at 20:47
2

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.

0

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
  • Given the usual conventions, I'd go a step farther, and write this as something like: if (!isFileCreated(handle = createFile()) log_error_and_quit(). Most modern filesystem APIs return some flavor of file handle, so that you can have multiple files open and manipulate them all, just by juggling the handles. And +1 for readability. – John R. Strohm Mar 26 '18 at 20:30
0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.