This code is taken from discourse code base, but similar code can be found in most of the applications.

def execute(args)
  raise Import::ImportInProgressError if Import.is_import_running?
  raise Export::ExportInProgressError if Export.is_export_running?

  @format = args[:format] || :json
  # additional initialization here

  start_export # start_export definition is below

  # business logic
end

def start_export
  if @format == :json
    @encoder = Export::JsonEncoder.new
  else
    raise Export::FormatInvalidError
  end
  # ...
end

There is a lot of code in this file (most was removed for clarity), but in general, the flow comes down to:

• Checking the precondition (and fail with one of Import::ImportInProgressError,  Export::ExportInProgressError, and Export::FormatInvalidError errors in case you can't start).

• Processing the export.

• Clearing the environment after the export.

At first glance, it may seem that everything is OK, but it's not. Some would say because "exceptions are expensive" (in terms of performance), which is true but this is not why. In general, when analyzing a process that you didn't write, usually the flow is:

1. Browse the code and read the documentation, if any. Usually, there is no documentation, so you end up browsing the code (or ask a colleague if possible).

2.  Check what the input data are.

3.  Check what the output data are.

This code actually breaks the third point. If you had a client with this code and you wanted to cover failing scenarios, you would have to write something like this:

def client_that_runs_execute
    data = prepare_data
    result = process.execute(data)
    process_result(result)
rescue Import::ImportInProgressError, Export::ExportInProgressError, Export::FormatInvalidError => e
    handle_export_not_processed_error(e)
rescue => e
    handle_unexpected_export_error(e)
end

So, you no longer have one kind of output. It's either the process result or errors.

Whether it's a problem or not depends on what is the logic behind the handle_export_not_processed_error . The more complex error handler, the more difficult code will be to maintain. If you write tests, you force yourself to mock `execute` and raise an exception. Then you have to check whether appropriate steps were taken. The business logic has a tendency to become more and more complex. Approaching with errors will make it harder to develop it.

Additionally, if you have more than one client using this code, each has to implement separate rescue methods (tests irritate even more).

Lastly, exceptions describe the truly exceptional situation. Does ExportInProgressError  really represent that?

Solution

OK, but what can we do instead? In our example, all the errors are thrown before the processing begins, in precondition checking step. I would suggest creating separate validator which decides whether we may start processing or not. If we can't (preconditions failed) then the whole process stops and exits with an error code/message. An example below:

module ExportValidator
    SUPPORTED_FORMATS = Set.new([:json])

    def self.can_start?(format) 
        errors = [] 
        errors << "another import is running" if Import.is_import_running? 
        errors << "another export is running" if Export.is_export_running? 
        unless SUPPORTED_FORMATS.include?(format) 
            errors << "unsupported format: #{format}" 
        end 
    errors 
    end 
end

def execute(args) 
    @format = args[:format] || :json 
    errors = ExportValidator.can_start?(@format)
    return error_result(errors) unless errors.empty?

    start_export 
end

def error_result(errors) 
    res = ExportResult.new 
    res.success = false 
    res.errors = errors 
    res 
end

Not Dogma

Can we make any exception to don't-control-flow-with-exceptions rule? Of course we can. This is not dogma, but I personally try not to overuse them. In my opinion, the deeper the layer (service, persistence) the fewer number of exceptions should be used because it's harder (thus more error prone, and more expensive in terms of time [money]) to change the code because the layers above already depends on this behavior, and you may not be able to detect all the scenarios that require changes (regression should be expected). On the other hand, if the application is relatively small, refactoring it may not be a problem.