Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I often find myself returning a boolean from a method, that's used in multiple locations, in order to contain all the logic around that method in a single place. All the (internal) calling method needs to know is whether the operation was successful, or not.

I'm using Python but the question isn't necessarily specific to that language. There are only two options I can think of

  1. Raise an exception, though the circumstances are not exceptional, and remember to catch that exception in every place the function is called
  2. Return a boolean as I'm doing.

This is a really simple example that demonstrates what I'm talking about.

import os

class DoSomething(object):

    def remove_file(self, filename):

        try:
            os.remove(filename)
        except OSError:
            return False

        return True

    def process_file(self, filename):

        do_something()

        if remove_file(filename):
            do_something_else()

Although it's functional, I really dislike this manner of doing something, it "smells", and can sometimes result in a lot of nested ifs. But, I can't think of a simpler way.

I could turn to a more LBYL philosophy and use os.path.exists(filename) prior to attempting deletion but there's no guarantees the file won't have been locked in the meantime (it's unlikely but possible) and I still have to determine whether the deletion has been successful or not.

Is this an "acceptable" design and if not what would be a better way of designing this?

share|improve this question
add comment

4 Answers

You should return boolean when the method/function is useful in making logical decisions.

You should throw an exception when the method/function isn't likely to be used in logical decisions.

You have to make a decision about how important the failure is, and if it should be handled or not. If you could classify the failure as a warning, then return boolean. If the object enters a bad state that make future calls to it unstable, then throw an exception.

Another practice is to return objects instead of a result. If you call open, then it should return a File object or null if unable to open. This ensures the programmers has an object instance that is in a valid state that can be used.

EDIT:

Keep in mind that most languages will discard the result of a function when it's type is boolean or integer. So it's possible to call the function when there is no left hand assignment for the result. When working with boolean results, always assume the programmer is ignoring the returned value and use that to decide if it should rather be an exception.

share|improve this answer
    
It's a validation of what I'm doing so I like the answer :-). On objects, though I understand where you're coming from I don't see how this helps in most of the cases I would use it. I want to be DRY so I'm going to be retuning the object to a single method as I only want to do one thing to it. I'm then left with the same code I have now, save with an additional method. (also for the example given I'm deleting the file so a null file object doesn't say much :-) –  Ben May 16 '13 at 16:44
    
Delete is tricky, because it's not guaranteed. I've never seen a file delete method throw an exception, but what can the programmer do if it fails? Continuously loop retrying? No, it's an OS issue. The code should log the result and move on. –  Mathew Foscarini May 16 '13 at 16:49
add comment

Your intuition on this is correct, there is a better way to do this: monads.

What Are Monads?

Monads are (to paraphrase Wikipedia) a way of chaining operations together while hiding the chaining mechanism; in your case the chaining mechanism is the nested ifs. Hide that and your code will smell much nicer.

There are a couple of monads that will do just that ("Maybe" and "Either") and lucky for you they're part of a really nice python monad library!

What they can do for your code

Here's an example using the "Either" monad ("Failable" in the library linked to), where a function can return a Success or Failure, depending on what occurred:

import os

class DoSomething(object):

    def remove_file(self, filename):
        try:
            os.remove(filename)
            return Success(None)
        except OSError:
            return Failure("There was an OS Error.")

    @do(Failable)
    def process_file(self, filename):
        do_something()
        yield remove_file(filename)
        do_something_else()
        mreturn(Success("All ok."))

Now, this might not look much different from what you have now, but consider how things would be if you had more operations that might result in a Failure:

    def action_that_might_fail_and_returns_something(self):
        # get some random value between 0 and 1 here
        if value < 0.5:
            return Success(value)
        else:
            return Failure("Bad value! Bad! Go to your room!")

    @do(Failable)
    def process_file(self, filename):
        do_something()
        yield remove_file(filename)
        yield action_that_might_fail(somearg)
        yield another_action_that_might_fail(someotherarg)
        some_val = yield action_that_might_fail_and_returns_something()
        yield something_that_used_the_return_value(some_val)
        do_something_else()
        mreturn(Success("All ok."))

At each of the yields in the process_file function, if the function call returns a Failure then the process_file function would exit out, at that point, returning the Failure value from the failed function, instead of continuing on through the rest and returning the Success("All ok.")

Now, imagine doing the above with nested ifs! (How would you handle the return value!?)

Conclusion

Monads are nice :)


Notes:

I'm not a Python programmer -- I used the monad library linked to above in a script I ninja'd for some project automation. I gather, though, that the preferred, idiomatic approach is to use exceptions.

IIRC there's a typo in the lib script on the page linked to though I forget where it is ATM. I'll update if I remember.

In order to get the result of a Failable decorated function (such as process_file) you have to capture the result in a variable and do a variable.value to get it.

share|improve this answer
add comment

A function is a contract, and its name should suggest what contract it will fulfil. IMHO, if you name it remove_file so it should remove the file and failing to do so should cause an exception. On the other hand, if you name it try_remove_file, it should "try" to remove and return boolean to tell if the file has been removed or not.

This would lead to another question -- should it be remove_file or try_remove_file? It depends on your call site. Actually, you can have both methods and use them in different scenario, but I think removing file per-se has high chance of success so I prefer having only remove_file that throw exception when fail.

share|improve this answer
add comment

In this particular case it may be useful to think about why you may not be able to remove the file. Let's say the problem is that the file may or may not exist. Then you should have a function doesFileExist() that returns true or false, and a function removeFile() that just deletes the file.

In your code you would first check if the file exists. If it does, call removeFile. If not, then do other stuff.

In this case you may still want removeFile to throw an exception if the file cannot be removed for some other reason, such as permissions.

To summarize, exceptions should be thrown for things that are, well, exceptional. So if it is perfectly normal that the file you are trying to delete may not exist, then that is not an exception. Write a boolean predicate to check for that. On the other hand, if you do not have the write permissions for the file, or if it is on a remote file system that is suddenly inaccessible, those may very well be exceptional conditions.

share|improve this answer
    
That's very specific to the example I've given, which I'd rather avoid. I've not yet written this, it's going to archive files and log the fact that this has happened in the database. Files can be reloaded at any time (though once loaded files are far less likely to be re-loaded) it's possible that a file can be locked by another process between the check and the deletion attempt. There's nothing exceptional about a failure. It is standard Python to not bother checking first and catch the exception when raised (if required), I simply don't want to do anything with it this time. –  Ben May 16 '13 at 21:21
    
If there is nothing exceptional about the failure, then checking whether you can remove a file is a legitimate part of your program logic. The single responsibility principle dictates that you should have a check function and a removeFile function. –  Dima May 17 '13 at 12:54
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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