3
\$\begingroup\$

I'm looking for advice on how to find a balance between keeping this code flexible/maintainable, while also keeping it readable.

It reads MQTT messages in JSON format from four different topics, converts the JSON into a different JSON format, and publishes it to a common Rabbit message queue. One of the topics, the sensor status topic, has messages in four different formats (four different sensor types publish their status in that topic).

In its original form the code is very straightforward and contained in a single class, however the class grows in size for each new sensor or topic added to it. My first decision to improve it was to create two message handler factories, one for the four topics and one for the special sensor topic, but then I thought using only one factory for both was better.

The bad thing about the factory is that it must be injected with a lot of arguments in order to perform its duty, which is to pass those arguments to the instantiated message handlers. I don't know how can I improve that. It seems to be a limitation of the Simple Factory "pattern" (not a real pattern though).

Two real patterns have also been applied to the code: Template Method (in ConvertingPublisherHandler) and Decorator (implemented in LoggingRabbitPublisher and used in ConvertingPublisherHandler).

Regarding Decorator, it is my first time applying it and I'm not sure if it is the best solution to the problem. I wanted to log both the original MQTT message and the converted JSON message in pairs, along with a line separator, so this is what the decorator does. This, however, caused the factory to receive a lot of arguments thus it is not very readable.

Regarding Template Method, I'm not sure if it is the most flexible solution to adopt. Firstly because the pattern is not very flexible by itself (it relies on inheritance and also freezes the sequence of methods it calls). Secondly because I took advantage of the common behavior for all messages that is to convert them into a different JSON and publish them in the message queue. But this could presumably change in the future (not every message handling could follow that behavior) and the template method would not be applicable to such scenarios.

Pattern overuse? I'm not a fan of randomly applying patterns, I just thought those fitted the situation. I don't usually apply so many patterns in such a short piece of code.

I consider the publisher a separate layer of the application (pretty much as a Data Access Object would belong to a separate layer) and so I had JsonPublisher throw JsonPublishingException. This exception encapsulates the real exceptions that the Rabbit publisher throws (IOException, TimeoutException, and InterruptedException). Please let me know if I'm in the right path.

I'm not including the whole list of message handlers, only OpticalSensorHandler.java. The others are similar.

Sorry about the English/Portuguese mix in the names. Any feedback is appreciated.

Main.java

private static void handleMessage(String topic, MqttMessage mensagem) throws MqttException, Exception {

    JsonMessageHandler handler;
    try {
        JSONObject json = new JSONObject(new String(mensagem.getPayload(), MQTT_MESSAGES_CHARSET));
        handler = jsonMessageHandlerFactory.newInstance(topic, json, mensagem, MQTT_MESSAGES_CHARSET);
    } catch (JSONException e) {
        logHighlighted(e);
        return;
    }

    if (handler == null) {
        logHighlighted("Mensagem desconhecida. Não será tratada.");
        return;
    }

    handler.handle();
}

JsonMessageHandler.java

public interface JsonMessageHandler {
    void handle() throws Exception;
}

JsonMessageHandlerFactory.java

public class JsonMessageHandlerFactory {

    private static final String SENSOR_STATUS_TOPIC_REGEX = "monitorada\\/[0-9]+\\/status\\-do\\-sensor\\/[0-9]+";

    private final JsonPublisher publisher;
    private final Charset destCharset;
    private final Logger logger;

    public JsonMessageHandlerFactory(JsonPublisher publisher, Charset destCharset, Logger logger) {
        this.publisher = publisher;
        this.destCharset = destCharset;
        this.logger = logger;
    }

    public JsonMessageHandler newInstance(String topic, JSONObject json, MqttMessage originalMsg, Charset origCharset) throws JSONException {
        if (topic.matches(PartitionStatusHandler.TOPIC_REGEX)) {
            return new PartitionStatusHandler(topic, publisher, originalMsg, origCharset, destCharset, logger);
        } else if (topic.matches(SENSOR_STATUS_TOPIC_REGEX)) {
            return createSensorStatusHandler(json, topic, originalMsg, origCharset, destCharset);
        } else if (topic.matches(SensorSupervisionHandler.TOPIC_REGEX)) {
            return new SensorSupervisionHandler(publisher, topic, originalMsg, origCharset, destCharset, logger);
        } else if (topic.matches(AlarmEventHandler.TOPIC_REGEX)) {
            return new AlarmEventHandler(topic, publisher, originalMsg, origCharset, destCharset, logger);
        } else {
            return null;
        }
    }

    private JsonMessageHandler createSensorStatusHandler(JSONObject json, String topic, MqttMessage originalMsg, Charset origCharset, Charset destCharset) throws JSONException {

        int classe = json.getInt("classe");
        int modelo = json.getInt("modelo");

        if (isOpticalSensorStatus(classe, modelo)) {
            return new OpticalSensorHandler(publisher, topic, originalMsg, origCharset, destCharset, logger);
        } else if (isMagneticSensorStatus(classe, modelo)) {
            return new MagneticSensorHandler( publisher, topic, originalMsg, origCharset, destCharset, logger);
        } else if (isIvpSensorStatus(classe, modelo)) {
            return new IvpSensorHandler(publisher, topic, originalMsg, origCharset, destCharset, logger);
        } else if (isSmartPlugSensorStatus(classe, modelo)) {
            return new SmartPlugSensorHandler(publisher, topic, originalMsg, origCharset, destCharset, logger);
        } else {
            return null;
        }
    }

    private static boolean isOpticalSensorStatus(int classe, int modelo) {
        return classe == 1 && modelo == 1;
    }

    private static boolean isMagneticSensorStatus(int classe, int modelo) {
        return classe == 3 && modelo == 1;
    }

    private static boolean isIvpSensorStatus(int classe, int modelo) {
        return classe == 4 && modelo == 1;
    }

    private static boolean isSmartPlugSensorStatus(int classe, int modelo) {
        return classe == 5 && modelo == 1;
    }
}

OpticalSensorHandler.java

public class OpticalSensorHandler extends ConvertingPublisherHandler {

    public OpticalSensorHandler(JsonPublisher publisher, String topic, MqttMessage originalMsg, Charset origCharset, Charset destCharset, Logger logger) {
        super(publisher, topic, originalMsg, origCharset, destCharset, logger);
    }

    @Override
    protected JSONObject convert(JSONObject mqttMsg) {
        JSONObject mppMsg = new JSONObject();
        mppMsg.put("tipoPacote", MppPacketType.STATUS_PORTAO.getNumber());
        JSONObject jsonSpirit = new JSONObject();
        jsonSpirit.put("idHome", mqttMsg.getInt("home"));
        jsonSpirit.put("particao", 0);
        mppMsg.put("spirit", jsonSpirit);
        JSONObject jsonSensor = new JSONObject();
        jsonSensor.put("idSensor", mqttMsg.getInt("id"));
        jsonSensor.put("aberto", mqttMsg.getInt("status") == 1);
        jsonSensor.put("calibradoAberto", mqttMsg.getInt("calibrado") == 2 || mqttMsg.getInt("calibrado") == 3);
        jsonSensor.put("calibradoFechado", mqttMsg.getInt("calibrado") == 1  || mqttMsg.getInt("calibrado") == 3);
        jsonSensor.put("sinalZigbee", mqttMsg.getInt("sinal-zigbee"));
        jsonSensor.put("temperatura", mqttMsg.getDouble("temperatura"));
        jsonSensor.put("bateriaFraca", mqttMsg.getInt("bateria") == 1);
        jsonSensor.put("falhaSupervisao", mqttMsg.getInt("falha-de-supervisao") == 1);
        mppMsg.put("sensor", jsonSensor);
        return mppMsg;
    }
}

ConvertingPublisherHandler.java

public abstract class ConvertingPublisherHandler implements JsonMessageHandler {

    private final JsonPublisher publisher;
    private final String topic;
    private final MqttMessage originalMsg;
    private final Charset origCharset;
    private final Charset destCharset;
    private final Logger logger;

    public ConvertingPublisherHandler(JsonPublisher publisher, String topic, MqttMessage originalMsg, Charset origCharset, Charset destCharset, Logger logger) {
        this.publisher = publisher;
        this.topic = topic;
        this.originalMsg = originalMsg;
        this.origCharset = origCharset;
        this.destCharset = destCharset;
        this.logger = logger;
    }

    protected abstract JSONObject convert(JSONObject json);

    @Override
    public void handle() throws Exception {
        JsonPublisher decoratedPublisher = new LoggingRabbitPublisher(publisher, topic, originalMsg, destCharset, logger);
        JSONObject originalJsonMsg = new JSONObject(new String(originalMsg.getPayload(), origCharset));
        decoratedPublisher.publish(convert(originalJsonMsg));
    }
}

LoggingRabbitPublisher.java

public class LoggingRabbitPublisher implements JsonPublisher {

    private final JsonPublisher publisher;
    private final String topic;
    private final MqttMessage originalMsg;
    private final Charset charset;
    private final Logger logger;

    public LoggingRabbitPublisher(JsonPublisher publisher, String topic, MqttMessage originalMsg, Charset charset, Logger logger) {
        this.publisher = publisher;
        this.topic = topic;
        this.originalMsg = originalMsg;
        this.charset = charset;
        this.logger = logger;
    }

    @Override
    public void publish(JSONObject jsonToPublish) throws JsonPublishingException {
        String payload = new String(originalMsg.getPayload(), charset);
        logger.println("---");
        logger.println("Tópico: " + topic + " QoS:" + originalMsg.getQos() + " " + payload);
        publisher.publish(jsonToPublish);
        logger.println("Publicado: " + jsonToPublish.toString());
    }
}

RabbitPublisher.java

public class RabbitPublisher implements JsonPublisher {

    private final RmqPublisher publisher;
    private final Charset charset;

    public RabbitPublisher(RmqPublisher publisher, Charset charset) {
        this.publisher = publisher;
        this.charset = charset;
    }

    @Override
    public void publish(JSONObject json) throws JsonPublishingException {
        String jsonString = json.toString();
        try {
            publisher.publish(jsonString.getBytes(charset));
        } catch (IOException | InterruptedException | TimeoutException e) {
            throw new JsonPublishingException(e);
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

Just picking out your factory problem (btw: all in all the code looks really good!):

Basically, you pass the same values around from instantiation of your factory to all the sub-objects, which in turn use these values, which makes for long and unwieldy parameter lists. Two solutions come to mind:

Classic:

Create a parameter object, which holds all parameters, instantiate that object, and pass it around instead (see https://refactoring.com/catalog/introduceParameterObject.html)

JEE (preferred, this is 2017 after all ;-)):

If you have the possibility to run in a CDI container (or maybe already do?), you can have various layers of factory object and automatically @Inject the constant parameters you need. If you have some kind of session handling, you can also inject that. Furthermore, with a CDI-container you can also cover the logging aspect with a logging interceptor so that your code does not have to care about that. Have a look at https://docs.oracle.com/javaee/7/tutorial/cdi-basic.htm for a starter, and a look at http://weld.cdi-spec.org/documentation/ for a reference implementation which can also work standalone.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the answer and the compliment! I managed to simplify the factory by changing the IS-A relationship in the ConvertingPublisherHandler to HAS-A (I.e. favor composition over inheritance) and make it receive a JsonConverter (so the handler is always the same class and the factory now creates JsonConverters. It is now very simple but still prone to receiving longer parameter lists so your solution is still in order. However I have issues and i'm stuck in the 90's, resistant to apply DI while its advantages don't strike me as obvious and I get rid of this feeling that it does "magic". \$\endgroup\$ – Piovezan May 10 '17 at 22:18

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.