Is it correct for a method to make the same exception for two different reasons?

Question

I’m practicing TDD simulating an alarm system. Alarm centers work connected to sensors that detect intrusion (opening a door or window, or movement within a room, for example). They have a fixed number of logical partitions (representing different locations to be protected) and slots for sensors. At the installation the sensors are associated to the respective partitions. Then the interface of my alarm center includes the following methods (Obs.: " arming" a partition means protecting it, i.e., the center will trigger the alarm if a partition sensor is triggered, and "disarm" means unprotected, i.e., ignore the drive of the partition sensors):

public interface InterfaceDeCentralDeAlarme {
    boolean armarParticao(int numero) throws IllegalArgumentException, IllegalStateException;
    boolean desarmarParticao(int numero) throws IllegalArgumentException, IllegalStateException;
    void associarSensorAParticao(int sensor, int particao) throws IllegalArgumentException, IllegalStateException;
    void desassociarSensorDeParticao(int sensor, int particao) throws IllegalArgumentException, IllegalStateException;
    ...
}

Here are some of the possible unit tests:

@Test(expected=IllegalStateException.class)
public void particaoSemSensoresAssociadosNaoPodeArmar() {
    InterfaceDeCentralDeAlarme central = criarCentralDeAlarme();
    central.armarParticao(1);
}

@Test(expected=IllegalStateException.class)
public void particaoJaArmadaNaoPodeArmar() {
    InterfaceDeCentralDeAlarme central = criarCentralDeAlarme();
    central.associarSensorAParticao(1, 1);
    central.fecharSensor(1);
    Assert.assertTrue(central.armarParticao(1));
    central.armarParticao(1);
}

The method armarParticao() delegates the arme to a class called Particao. So far, no big deal:

@Override
public boolean armarParticao(int numero) throws IllegalArgumentException, IllegalStateException {
    Particao particao = particoes.get(numero);
    if (particao == null) {
        throw new IllegalArgumentException();
    }

    return particao.armar();
}

In class Particao created a public method armar() that can cast IllegalStateException for two different reasons, that is, two different situations in which the object is in an improper state. One is when the partition is already armed; another is when the partition has no sensors associated with it (i.e., there is no way to detect intrusion):

public class Particao {

    private final Map<Integer, Sensor> sensores = new HashMap<>();
    private boolean armada = false;

    public boolean armar() throws IllegalStateException {
        if (armada) {
            throw new IllegalStateException("Partição já se encontra armada");
        }

        if (sensores.isEmpty()) {
            throw new IllegalStateException("Partição não possui sensor associado");
        }

        for (Sensor sensor : sensores.values()) {
            if (sensor.isAberto() && false == sensor.isInibido()) {
                return false;
            }
        }

        this.armada = true;
        return true;
    }

    ...
}

From the point of view of unit tests and TDD, is that bad? A colleague said yes, because if the method launches a IllegalStateException I’ll never be sure if it’s because of one reason or because of the other. The exception thrown may mask a different situation than the one I’m trying to test.

But I’m not sure that’s a problem. Maybe it is if I don’t have tests for every possible situation (including the two situations where IllegalStateException can be launched). But if I have, maybe this overlap doesn’t get to be a problem. In practice, when writing unit tests or doing TDD people have the habit of avoiding it (throw the same exception in a method in two different situations)?

Second question: if I have to use different exceptions, should I create custom exceptions (new classes) for each of the situations, or is this a more or less arbitrary decision? A(s) exception(s) created(s) shall be subclasses of IllegalStateException?

Answer 1

Question modified after answer

Everything in engineering needs to be thought out for what you’re doing. For once the answer is a huge DEPENDS.

The first thing is to ask yourself if it will really make a difference to have different situations. Often you just need to know that the exception occurred, you don’t need to know the exact situation, so you can live with it. If that’s the case, you could, but not necessarily, rewrite it like this:

 if (condicao1 || condicao2) throw new IllegalStateException("Falhou devido à condição");

The test needs to be adequate

If you cannot make your life easier, one of the solutions is to take the test to identify the details of the exception, if this is important.

Example:

try {
    executarAcao();
} catch (Exception e) {
    assertThat(e).isInstanceOf(IllegalStateException.class)
                 .hasMessage("Falhou devido à condição 1");
}

I put in the Github for future reference.

Junit has the expectMessage() for this. I believe that most of the good frameworks have similar facilities.

You can also use annotation expected=IllegalStateException.class, message = "Falhou devido à condição 1".

It is normal for a method to bid more than one equal exception and need to be tested, so there are facilities to test. If the test needs to do this, do it and test it properly.

The method is wrong

Another solution is to release different, more customized exceptions.

There are those who say that this should always be done, that generic exceptions are not good. Others think exaggeration. It depends on the case. If you are going to change a small detail, it may be too much, but if it is unrelated, then it may be the case. You may be using a generic exception for "sloth". Remember that there are people who only throw Exception, then the problem is another. Launch IllegalStateException may fall into the same problem even though it is not so apparent. It may also be the right one for the case.

Whether they should be subclasses or not depends on the case. This may be one that should, by the looks of it, but of course it is a very hypothetical example. I wouldn’t say it’s an arbitrary decision, it’s an individual decision.

Note that the problem there is of design method*. If so, solve this problem. The previous solution is to solve the design test, which should be preferred if your method is correct.

After editing the question you can see that the method has different problems. At least one of them is not an illegal state, it’s just a state that prevents the operation. Which points out that the exception is being used as business rule (I understand that it is common in Java to do this, but I would go for another way). The other may, but not necessarily, exist by a class failure, perhaps there should be the object with illegal state. If everything is right, the exceptions are completely different. The error is not to hinder the test, it is wrong specification. But I may have misinterpreted.

I find it absurd that people change their primary code to meet a test demand. You must allow the test, eventually to facilitate, but never at the cost of the primary responsibility of your code. There are those who disagree, but I find it a mistake to program for the test. Do something testable, ok, but the code must be done to meet the specification.

Completion

As far as I know people do not avoid throwing the same exception no, if the method needs, should do. And I see as something very common. I don’t know if you have any better solutions.

The question example shows how TDD is a complicated thing. It works well when: (a) the problem is widely known and has very good specification (rare); (b) an architectural genius did it.

Were you curious to ask the colleague what the solution is? Maybe he doesn’t know what he’s talking about, even if :). Remember that many people read about something being "good or bad practice" somewhere, repeat what they "saw", but do not understand the problem. Obviously I don’t know if that’s the case.