Is it correct to throw the same exception for two different reasons?

Question

Is it correct to throw the same exception for two different reasons?

Navigation

#1 by (9 votes)

11

I'm practicing TDD simulating a central alarm. Alarm centrals work connected to sensors that detect intrusion (opening a door or window, or moving inside a room, for example). They have a fixed number of logical partitions (representing different locations to be protected) and slots for sensors. At the time of installation the sensors are associated with the respective partitions. So the interface of my alarm center includes the following methods (note: "arm" a partition means to protect it, that is, the control panel will trigger the alarm if a partition sensor is triggered, and "disarm" means unprotect, ie ignoring 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 armarParticao() method delegates the arming to a class named Particao . Until then, nothing too much:

@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 I created a public method armar() that can cast IllegalStateException for two distinct reasons, that is, two different situations where the object is in an undue state. One is when the partition is already armed; another is when the partition does not have sensors associated with it (that is, it can not 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 testing and TDD, is this a bad thing? A colleague said yes, because if the method throws a IllegalStateException I'll never be sure if it's because of one reason or because of another. The thrown exception may mask a different situation than the one I'm trying to test.

But I'm not sure if that's ever a problem. Maybe it is if I do not have tests for all possible situations (including the two situations where% of% can be posted). But if I do, maybe this overlap will not be problematic. In practice, when writing unit tests or doing TDD, people have the habit of avoiding this (throwing the same exception in one method in two different situations)?

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

java tdd

asked by anonymous 03.07.2016 / 00:44

1 answer

How to get the last working day of the month? Intersection area of two polygons

score 9 · Accepted Answer

The question was modified after the answer

Everything in engineering needs to be thought for what it is doing. To vary the answer is a huge DEPENDE.

The first thing to ask yourself is whether it will really make a difference to have different situations. Many times you just need to know that the exception has occurred, you do not need to know the exact situation, so you can live with it. If so, it may be, but not necessarily, that I could rewrite it:

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

The test must be adequate

If it can not make your life easier, one of the solutions is to have the test identify the details of the exception if that is important.

Example:

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

Junit has expectMessage() for this. I believe 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 throw more than one exception equal and need to be tested, so there are facilities to test. If the test needs to do this, do and test it appropriately.

The method is wrong

Another solution is to throw different, more personalized exceptions.

Some people say that this should always be done, that generic exceptions are not good. Others find it exaggerated. Depends on the case. If it is to change a small detail, it may be an exaggeration, but if it is an unrelated thing, then it may be the case. You may be using a generic exception for "laziness." Remember that there are people who only throw Exception , there the problem is another. Casting IllegalStateException may fall into the same problem, even though it is not so apparent. It may also be the right thing to do.

Whether they should be subclasses or not, depends on the case. This may be one that must, for what it seems, but of course it is a very hypothetical example. I would not say it's an arbitrary decision, it's an individual decision.

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

After editing 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 a business rule (I understand that it is common in Java to do this, but I would go by another path ). The other may, but need not, exist because of a class failure, perhaps there should not be an object with illegal status. If all is right, the exceptions are completely different. The error is not making the test difficult, it is wrong specification. But I may have misinterpreted it.

I find it absurd for people to change their primary code to meet a test demand. You should allow the test eventually to ease, 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 made to meet the specification. If TDD is the specification, it is wrong.

Conclusion

As far as I know people do not shy away from throwing the same exception, not if method needs, should do. And I see it as something very common. I do not know if you have any better solutions.

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

Have you been curious to ask your colleague what the solution is? Maybe he does not know what he's talking about. Sounds right. Remember that many people read about something being "good or bad" somewhere, repeating what they "saw", but do not understand the problem. Obviously I do not know if that's the case.