Returning an object. Any Smell code here?

Question

I thought of the following public method:

public RespostaDeArme armar();

The class RespostaDeArme would be so:

public class RespostaDeArme {

    private final ResultadoDeArme resultado;
    private final Set<Integer> zonasAbertas;

    public RespostaDeArme(ResultadoDeArme resultado) {
        this(resultado, new HashSet<>());
    }

    public RespostaDeArme(ResultadoDeArme resultado, Set<Integer> zonasAbertas) {
        this.resultado = resultado;
        this.zonasAbertas = new HashSet<>(zonasAbertas);
    }

    public ResultadoDeArme getResultado() {
        return resultado;
    }

    public Set<Integer> getZonasAbertas() {
        return zonasAbertas;
    }
}

public enum ResultadoDeArme {
    SUCESSO(1),
    ARME_RECUSADO_PARTICAO_JA_ARMADA(2),
    ARME_RECUSADO_EXISTEM_ZONAS_ABERTAS(3),
    ERRO(4);

    private final int numero;

    private ResultadoDeArme(int numero) {
        this.numero = numero;
    }

    public int getNumero() {
        return numero;
    }
}

It only makes sense to call the method getZonasAbertas() when the result is ARME_RECUSADO_EXISTEM_ZONAS_ABERTAS.

Question: does this code have any code Smell? The only one I thought was an eventual need to refactor the class containing the method armar() with Hide Delegate not to need the intermediary RespostaDeArme to get the open zones (which I don’t know if would be the case here).

Does anyone see a better way to implement this behavior? If necessary I give more context. I don’t like the idea of having two methods in the same class, one armar() and a getZonasAbertas(), because the two would have an unnecessary temporal link (the programmer would need to know that he should first call the armar(), then the other).

Example of use:

public void executarArme(int idDaCentral) {
    RespostaDeArme resposta = conexao.armar();
    escritorDaFila.escrever(gerarJsonDeRespostaDeArme(idDaCentral, resposta));
}

private JSONObject gerarJsonDeRespostaDeArme(int idDaCentral, RespostaDeArme resposta) {

    JSONObject json = new JSONObject();

    json.put("tipoDeMensagem", Mensagens.RESPOSTA_DE_ARME.getNumero());
    json.put("idDaCentral", idDaCentral);
    json.put("resultado", resposta.getResultado().getNumero());

    if (resposta.getResultado() == ResultadoDeArme.ARME_RECUSADO_EXISTEM_ZONAS_ABERTAS) {
        json.put("zonasAbertas", new JSONArray(resposta.getZonasAbertas()));
    }

    return json;
}

Answer 1

Now that you’ve edited the question, it can be solved with a simple Optional, hiding the constructor and exposing static methods of Factory:

public final class RespostaDeArme {

    private final ResultadoDeArme resultado;
    private final Optional<Set<Integer>> zonasAbertas;

    public static RespostaDeArme sucesso() {
        return new RespostaDeArme(ResultadoDeArme.SUCESSO, Optional.empty());
    }

    public static RespostaDeArme armeFalhou() {
        return new RespostaDeArme(ResultadoDeArme.ERRO, Optional.empty());
    }

    public static RespostaDeArme particaoJaArmada() {
        return new RespostaDeArme(
                ResultadoDeArme.ARME_RECUSADO_PARTICAO_JA_ARMADA,
                Optional.empty());
    }

    public static RespostaDeArme existemZonasAbertas(Set<Integer> zonasAbertas) {
        return new RespostaDeArme(
                ResultadoDeArme.ARME_RECUSADO_EXISTEM_ZONAS_ABERTAS,
                Optional.of(Collections.unmodifiableSet<>(new LinkedHashSet<>(zonasAbertas))));
    }

    private RespostaDeArme(ResultadoDeArme resultado, Optional<Set<Integer>> zonasAbertas) {
        this.resultado = resultado;
        this.zonasAbertas = new HashSet<>(zonasAbertas);
    }

    public ResultadoDeArme getResultado() {
        return resultado;
    }

    public int getNumeroResultado() {
        return resultado.getNumero();
    }

    public Optional<Set<Integer>> getZonasAbertas() {
        return zonasAbertas;
    }
}

public enum ResultadoDeArme {
    SUCESSO,
    ARME_RECUSADO_PARTICAO_JA_ARMADA,
    ARME_RECUSADO_EXISTEM_ZONAS_ABERTAS,
    ERRO;

    public int getNumero() {
        return ordinal() + 1;
    }
}

public void executarArme(int idDaCentral) {
    RespostaDeArme resposta = conexao.armar();
    escritorDaFila.escrever(gerarJsonDeRespostaDeArme(idDaCentral, resposta));
}

private JSONObject gerarJsonDeRespostaDeArme(int idDaCentral, RespostaDeArme resposta) {

    JSONObject json = new JSONObject();

    json.put("tipoDeMensagem", Mensagens.RESPOSTA_DE_ARME.getNumero());
    json.put("idDaCentral", idDaCentral);
    json.put("resultado", resposta.getNumeroResultado());

    resposta.getZonasAbertas().ifPresent(zonas -> {
        json.put("zonasAbertas", new JSONArray(zonas));
    });

    return json;
}

Answer 2

It depends a lot on what is the context in which the method armar() will be used. However, the way it is, I believe so, that there is actually a Smell code, although it should not be a serious Smell code. From what I understand, a RespostaDeArme who has a ResultadoDeArme other than SUCESSO indicates that the method armar() failed. The correct mechanism to denote such failures is the handling of exceptions.

So your code can look like this:

public class ArmeException extends Exception {}

public class ParticaoJaArmadaException extends ArmeException {}

public class ExistemZonasAbertasException extends ArmeException {

    private final Set<Integer> zonasAbertas;

    public RespostaDeArme(Set<Integer> zonasAbertas) {
        this.zonasAbertas = new HashSet<>(zonasAbertas);
    }

    public Set<Integer> getZonasAbertas() {
        return zonasAbertas;
    }
}

public class ArmeFalhouException extends ArmeException {}

And your method armar() gets like this:

public void armar() throws
        ParticaoJaArmadaException,
        ExistemZonasAbertasException,
        ArmeFalhouException;