3
\$\begingroup\$

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;
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ You did a fine job of asking your question, as originally posted. On Code Review, we encourage you to post your real code. Please do not feel pressured to alter your code for the sake of posting a question. And you should certainly not alter the code in response to an answer; that is strictly forbidden here. I've rolled back Rev 4. \$\endgroup\$ – 200_success Jul 16 '16 at 17:21
  • \$\begingroup\$ @200_success Thank you for the clarification. If the site's policy encourages posting real code regardless of the spoken language it is written in, I think this question on Code Review Meta should be updated accordingly. It is the only question I could find regarding dealing with foreign language and none of its answers mentions this, which is confusing. \$\endgroup\$ – Piovezan Jul 16 '16 at 17:53
  • \$\begingroup\$ @200_success Also regarding not altering the code in response to an answer, I suspect this may be referring to altering the structure of the code, which could be confusing to readers of the original question, and not to translating the identifiers, which is more of a cosmetic change. But I'm new to the site and could be wrong. \$\endgroup\$ – Piovezan Jul 16 '16 at 17:55
  • \$\begingroup\$ Unfortunately, the answer that was accepted by the person who asked that question was the minority opinion. \$\endgroup\$ – 200_success Jul 16 '16 at 17:55
  • \$\begingroup\$ No, we are quite strict on about not changing anything that would invalidate a point that was made in an answer. The whole concept of Code Review would fall apart if we allowed that. \$\endgroup\$ – 200_success Jul 16 '16 at 17:57
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Please drop the caps, it is really offensive. People can write their personal code any language they want (and the code here is not personal, it is corporate, and it is in Portuguese because this is a business requirement). If the site requires code to be in English I can just take the trouble to translate it as I did but lecturing me to always have my code in English is way off. Thank you. \$\endgroup\$ – Piovezan Jul 16 '16 at 15:56
  • \$\begingroup\$ @Piovezan I am really really sorry if I offended you. That wasn't my goal. My point is that it's a really bad practice, and I wanted to emphasize how important it is to not get used to writing non-english code. I'll drop the caps, if it's important to you. Though you should still remember it. I hope the rest of the answer will help you :) \$\endgroup\$ – Hexa Jul 16 '16 at 16:00
  • \$\begingroup\$ That's okay. Even though I'm able to switch languages depending on the codebase, I feel that practices in my native language allow me to think better about my design decisions, something that becomes somewhat more difficult when thinking/naming non-natively (to the point that I'm willing to go through the effort of translating that code when I look for help in a foreign site). I do appreciate your advice. I'm going to comment on it. \$\endgroup\$ – Piovezan Jul 16 '16 at 18:15
  • \$\begingroup\$ I do agree that disconnects are not really abnormal when dealing with remote networking. What bothers me is that the Java I/O API treats them as exceptions. Is that correct for their case and not for mine? Would have a different design of the I/O API been better? How do I tell? \$\endgroup\$ – Piovezan Jul 16 '16 at 18:41
  • \$\begingroup\$ @Piovezan just as I said, there is no correct way. It's just about the way you prefer to manage your code \$\endgroup\$ – Hexa Jul 16 '16 at 18:48

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.