Dealing with an Exception is One Thing

Nothing new, but I keep seeing this bad practice around again and again... Let's explain why this is incorrect: boolean save().


"Functions should do one thing. Error handling is one thing." -- Robert C. Martin (Clean Code)

This is a very simple principle, but I see it in action only rarely. What's actually wrong with that?:

class TextFile {
    private final Path path;
    
    TextFile(Path path) { this.path = path; }
    
    boolean save(String content) {
        try {
            byte[] bytes = content.getBytes("UTF-8"); 
            Files.write(this.path, bytes);
            
            return true;
            
        } catch (IOException e) {
            // log the exception
        }
        return false;
    } 
}

The thing here is we are trying to inform the client about the fact the save operation was successful or not. But this is not what the method is supposed to do, the method should save the content, period.

Exposing a boolean return value the method says to its client that the error is some kind of correct behavior. But the client doesn't expect such a behavior, the client simply expect the content to be saved. If not, it is an incorrect behaviour and an exception should occur.

The save method is different from, for example, a boolean readable() method. We expect this method to answer the question if the file is readable or not. And false is a valid answer in this case.

What's the right approach? If you prefer unchecked exceptions (I do), consider the following:

class TextFile {
    private final Path path;
    
    TextFile(Path path) { this.path = path; }
    
    void save(String content) {
        try {
            byte[] bytes = content.getBytes("UTF-8"); 
            Files.write(this.path, bytes);
            
        } catch (IOException e) {
            throw new FileNotSavedException("Cannot save a file: " + path, e);
        }
    } 
}

class FileNotSavedException extends RuntimeException {
    FileNotSavedException(String message, Throwable cause) {
        super(message, cause);
    }
} 

Now, the save method is doing exactly what we expect from it: saves a context in a file. When this isn't for whatever reason possible, an exception is thrown and it's up to the client to deal with the situation.

Have an exceptional day!