Command sequence executor with error handling

Question

I've recently learned that as a Java developer my methods throw exceptions way too often, even when the reason for throwing is related to a business rule, when returning an error code instead could perhaps be more suitable.

I'm trying to learn how to differentiate real exceptional cases which require throwing exceptions from the ones I don't need to. So I'd like this code review's main focus to be on avoiding unnecessary exceptions.

The code in Portuguese, but I'll do my best to explain it in English. What is going on in it: this server-side Java class has a BlockingQueue<JSONObject> holding objects representing a command sequence along with its parameters. Command sequences are taken from the queue in a loop and executed (they send commands to a remote device; a command is a request sent to the device which sends a response back). The executarSequenciaDeComandos() ("executeCommandSequence") method returns a result which updates the command sequence status in the database (it may either succeed, or fail due to a number of reasons). When there is no command sequence to execute, the thread blocks waiting for the next command sequence.

As long as there is a connection to the device, the thread loop is kept running, executing command sequences or waiting for the next one. If the connection is lost, an observer is notified which interrupts the thread, causing an InterruptedException to be thrown (I'm replacing it with a ViaDeComunicacaoFechadaException, which translates to ConnectionClosedException). Such an exception causes the loop to end. The executarSequenciaDeComandos() method can also throw such an exception in the middle of the communication with the device causing the same result.

My questions:

Should aguardarProximaSequenciaDeComandos() ("waitForNextCommandSequence") really throw ViaDeComunicacaoFechadaException? Is losing the connection a normal, expected scenario in this case or should I consider it an exceptional situation? Connections can be lost often, leading the device to attempt to reconnect to the server. Shouldn't this method just catch the InterruptedException and return null instead, and I include a null check after this method's call which causes the execution flow to break out of the loop?

If I consider it a normal situation, shouldn't I consider it normal as well regarding to the executarSequenciaDeComandos() method? In other words, shouldn't this method favor returning a status code indicating the the connection has been lost over throwing a ViaDeComunicacaoFechadaException? Currently I'm favoring the exception because the command sequences (not shown in the code) are algorithms which send a number of commands to the device and each command has the possibility of losing connection with the device, thus I end up with a simpler algorithm if I allow each one of those commands to throw an exception that will bubble up instead of having the result of each of them checked for a connection lost status, which would really pollute the algorithms.

For further clarification of how I communicate with the device from inside a command sequence algorithm, I have for instance a ConnectionWithGivenDevice class with an interface such as:

public ResultFromCommandA callCommandA(parameters) throws ViaDeComunicacaoFechadaException, InterruptedException;
public ResultFromCommandB callCommandB(parameters) throws ViaDeComunicacaoFechadaException, InterruptedException;
public ResultFromCommandC callCommandC(parameters) throws ViaDeComunicacaoFechadaException, InterruptedException;

I tried comparing those two scenarios with general I/O handling in Java but I'm not sure this is a good comparison. E.g. trying to read or write through a broken stream in Java throws an IOException. Which means that this is an exceptional condition. Can I say the same for my scenarios?

Finally, depending on the adopted solution I might be able to break the executarLoopDeExecucaoDeSequenciasDeComandos() ("runCommandSequencesExecutionLoop") method which contains the while loop into smaller methods. Currently I'm finding it a bit difficult due to the break line which prevents turning the inner try block into a new method. Or perhaps that does not depend upon the solution and can be solved in another way.

I was going to ask for recommendations on how to deal with the other exceptions, but I'm afraid the review would become unnecessarily long. Any to-the-point comments on them would be welcome though. DeveDerrubarOProcessoException translates to "ProcessMustBeShutdownException"; and InterruptedException is thrown if by any reason (currently none) the wait for a command response is interrupted (I suppose I'd want it to be another side effect of losing connection with the device).

public abstract class ExecutorDeSequenciasDeComandos {

    private final BlockingQueue<JSONObject> filaBloqueanteDeSequenciasDeComandos = new LinkedBlockingQueue<>();

    private final AtualizadorDeStatusDeSequenciasDeComandos atualizadorDeStatusDeSequenciasDeComandos;

    private final Thread threadExecutorDasSequenciasDeComandos = new Thread() {
        @Override
        public void run() {
            executarLoopDeExecucaoDeSequenciasDeComandos();
        }
    };

    public ExecutorDeSequenciasDeComandos(AtualizadorDeStatusDeSequenciasDeComandos atualizadorDeStatusDeSequenciasDeComandos) {
        if (atualizadorDeStatusDeSequenciasDeComandos == null) {
            throw new IllegalStateException("Os argumentos não podem ser nulos.");
        }

        this.atualizadorDeStatusDeSequenciasDeComandos = atualizadorDeStatusDeSequenciasDeComandos;
    }

    protected abstract ViaDeComunicacao getViaDeComunicacao();

    protected abstract void logarComIdDoPainel(String mensagem);

    protected abstract void logarComIdDoPainel(Exception e);

    public void chamarEsteMétodoUmaVezLogoApósInstanciação() {
        getViaDeComunicacao().adicionarObservadorDeFechamentoDeConexao(new ObservadorDeFechamentoDeViaDeComunicacao() {
            @Override
            public void onFechado(ViaDeComunicacao via) {
                logarComIdDoPainel("Conexão fechada.");
                threadExecutorDasSequenciasDeComandos.interrupt();
            }
        });

        threadExecutorDasSequenciasDeComandos.start();
    }

    public final void receberSequenciaDeComandos(JSONObject jsonComando) {
        if (getViaDeComunicacao().isAberta()) {
            filaBloqueanteDeSequenciasDeComandos.offer(jsonComando);
        }
    }

    private JSONObject aguardarProximaSequenciaDeComandos() throws ViaDeComunicacaoFechadaException {
        try {
            return filaBloqueanteDeSequenciasDeComandos.take();
        } catch (InterruptedException e) {
            throw new ViaDeComunicacaoFechadaException(e);
        }
    }

    protected abstract ResultadoDaSequenciaDeComandos executarSequenciaDeComandos(JSONObject jsonComando) throws
            ViaDeComunicacaoFechadaException, DeveDerrubarOProcessoException, InterruptedException;

    private void executarLoopDeExecucaoDeSequenciasDeComandos() throws DeveDerrubarOProcessoException, RuntimeException {
        while (true) {
            try {
                JSONObject jsonComando = aguardarProximaSequenciaDeComandos();
                Integer idDoComando = jsonComando.getInt("idComando");
                try {
                    atualizadorDeStatusDeSequenciasDeComandos.enviarParaFilaStatusDoComando(idDoComando, StatusDeSequenciaDeComandos.EM_EXECUCAO, null);
                    ResultadoDaSequenciaDeComandos resultado = executarSequenciaDeComandos(jsonComando);
                    atualizadorDeStatusDeSequenciasDeComandos.enviarParaFilaStatusDoComando(idDoComando, resultado.getStatus(), resultado.getDescricao());
                } catch (JsonSyntaxException e) {
                    atualizadorDeStatusDeSequenciasDeComandos.enviarParaFilaStatusDoComando(idDoComando, StatusDeSequenciaDeComandos.ERRO, "Erro lendo os parâmetros do JSON: " + e.getMessage());
                } catch (ViaDeComunicacaoFechadaException | InterruptedException e) {
                    atualizadorDeStatusDeSequenciasDeComandos.enviarParaFilaStatusDoComando(idDoComando, StatusDeSequenciaDeComandos.TIMEOUT, null);
                    break;
                } catch (DeveDerrubarOProcessoException e) {
                    throw e;
                } catch (RuntimeException e) {
                    logarComIdDoPainel(e);
                    atualizadorDeStatusDeSequenciasDeComandos.enviarParaFilaStatusDoComando(idDoComando, StatusDeSequenciaDeComandos.ERRO, "Ocorreu um erro ao executar o comando.");
                }
            } catch (JSONException e) {
                logarComIdDoPainel(e);
            } catch (ViaDeComunicacaoFechadaException e) {
                break;
            }
        }
    }
}

Answer 1

The best tip I can give you regarding to this question is: always code in english. English is the main, universal language of coding, and just like all programming languages, and this forum, are in english, so should be your code. I know that you are probably saying right now "but this code is for myself". It doesn't matter. As a programming practice, and also for situations like you asking a question on the internet, you should always keep your code in english.

Now that that's settled, we can move on to the actual answer.

Well, the short answer is, it depends. There isn't really a "right" way. It just depends on how exactly you want to manage the code. On one hand, it's correct to use an exception. It's a special case that the calling function needs to be notified of. In that case, you should have the calling function catch the exception and deal with it. On the other hand, exceptions are, well, exceptional. It should be used for stating abnormal behavior. And when dealing with wireless/remote networking, disconnects aren't that abnormal. So what can you do instead? Well, a very common programming trick in those cases is to make your method boolean. Then, you can return true to indicate success or false to indicate failure. I also personally think it's a the best option for this type of "gray-zone" exception like your situation.

I didn't have the time to completely read the code (or the will to deal with the fact that it's in Portuguese :P), so I'm not sure exactly what's returning what, but if it's important for your method to return a non-boolean value, then instead of the exception, you can return null, but again, make sure the caller deals with it.