HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Validate the inputs and perform actions accordingly

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewvalidationjavastackoverflowguava
validateinputstheactionsperformandaccordingly

Problem

I have a consumer which is listening to Kafka and from which I am getting avro GenericRecord.

  • Now basis on GenericRecord payload, I am extracting lot of other things. clientId, deviceId, payId, crossId, appPayload.



  • And on all those things I have to do sanity check to make sure they are null/empty or not.



  • And if they are null/empty, then I want to increment the metric counter for that particular metric, log it and continue to the next record from the for loop.



  • And if they are not null/empty, then I am populating all those fields into a DataPacket builder. All the methods in DataPacket builder have a Preconditions.checkArgument check for null/empty string so I just cannot set them directly without checking them first. If I don't check before setting into DataPacket it will throw exception which will get caught in a catch block and then the whole program will get stopped and it won't move to next record from the for loop as you can see below.



```
try {
GenericRecordDomainDataDecoder decoder = new GenericRecordDomainDataDecoder(config);
while (true) {
ConsumerRecords records = consumer.poll(1000);
for (ConsumerRecord record : records) {
DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
MetricName.TOTAL_NUMBER);
GenericRecord payload = decoder.decode(record.value());
String clientId = (String) DataUtils.parseRecord(payload, "clientId");
String deviceId = (String) DataUtils.parseRecord(payload, "deviceId");
// can this be improved in any way? the whole if null check for all the below cases?
if (clientId == null && deviceId == null) {
DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
MetricName.INVALID_ID);
logger.logError("dropping records. invalid clientId and deviceId is coming.);
continue;
}
String payId = (String) DataUtils.parseRecord(payload, "payId");
if (payId == null) {
DataMe

Solution

I think you have some great questions here!

You asked:


Is there any better way to do the above things so that the code looks clean?

I'm not sure that's your only consideration here because it looks like this code is designed to be as efficient as possible. I say that because of the fact that the validation logic skips over the record as soon as a problem is found and the packetBuilder is only created after the initial validation - thereby avoiding the cost of creating the object every time through the loop regardless of the validity of the data.

So, with all of this in mind I'm afraid that my answer to your question will have to be: there might be slight improvements you can make but you can't avoid the null checking unless the Exception thrown by the builder is very detailed about what exactly went wrong and you can use it to gather your metrics.

If you want to make the code more modular you could split the validation logic into separate methods - perhaps one for each field. I'm not sure exactly how efficient you're trying to be and there is some cost involved with calling methods, so this might not be a good change for you. However, if peak performance is required then I'm sure you're familiar with profiling so just use whatever tool you typically use and determine whether the change impacts performance.

Along the same line of thinking you might even create an object to hold the fields and provide methods for validating each of them as well as a method to validate the data in general (this method could call each of the field validation methods and determine a result based on the results for the fields). Note that this approach also has overhead cost as you would create this object each time through the loop.

Here is what the class to hold the fields might look like: 

public class PacketValidation {
    private String clientId, deviceId, payId, crossId;
    private Map appPayload;
    // Not sure how you initialize the logger, so just added it as a field.
    private Logger logger;

    public PacketValidation(GenericRecord payload){
        clientId = (String) DataUtils.parseRecord(payload, "clientId");
        deviceId = (String) DataUtils.parseRecord(payload, "deviceId");
        payId = (String) DataUtils.parseRecord(payload, "payId");
        crossId = (String) DataUtils.parseRecord(payload, "crossId");
        appPayload = (Map) DataUtils.parseRecord(payload, "appPayload");
    }

    public boolean isValid(){
        return isValidClientIdDeviceId() && isValidPayId() && isValidAppPayload();
    }

    private boolean isValidAppPayload() {
        if (MapUtils.isEmpty(appPayload)) {
            DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
                MetricName.INVALID_PAYLOAD);
            logger.logError("dropping records. invalid payload is coming.");
            return false;
          }
        return true;
    }

    private boolean isValidPayId() {
        if (payId == null) {
            DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
                MetricName.MISSING_PAY_ID);
            logger.logError("dropping records. invalid payId is coming.");
            return false;
          }
        return true;
    }

    private boolean isValidClientIdDeviceId() {
        if (clientId == null && deviceId == null) {
            DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
                MetricName.INVALID_ID);
            logger.logError("dropping records. invalid clientId and deviceId is coming.");
            return false;
          }
        return true;
    }

    public String getClientId() {
        return clientId;
    }

    public String getDeviceId() {
        return deviceId;
    }

    public String getPayId() {
        return payId;
    }

    public String getCrossId() {
        return crossId;
    }

    public Map getAppPayload() {
        return appPayload;
    }
}


Then your code might look something like:

```
try {
GenericRecordDomainDataDecoder decoder = new GenericRecordDomainDataDecoder(config);
while (true) {
ConsumerRecords records = consumer.poll(1000);
for (ConsumerRecord record : records) {
DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
MetricName.TOTAL_NUMBER);
GenericRecord payload = decoder.decode(record.value());

//Replaced validation logic with the following 3 lines:
PacketValidation validation = new PacketValidation(payload);
if(!validation.isValid())
continue;

DataMetricHolder.getInstance().increment(validation.getPayId(), MetricName.TOTAL_NUMBER);

Builder packetBuilder = new DataPacket.Builder(payload);
if (validation.getClientId() != null) {
packetBuilder.setClientId(validation.getClientId());
}
if (validation.getDeviceId() != null) {
packetBuilder.setDeviceId(validation.getDeviceId());
}
if (validation.getCrossId() != null) {
packetBuilder.s

Code Snippets

public class PacketValidation {
    private String clientId, deviceId, payId, crossId;
    private Map<String,String> appPayload;
    // Not sure how you initialize the logger, so just added it as a field.
    private Logger logger;

    public PacketValidation(GenericRecord payload){
        clientId = (String) DataUtils.parseRecord(payload, "clientId");
        deviceId = (String) DataUtils.parseRecord(payload, "deviceId");
        payId = (String) DataUtils.parseRecord(payload, "payId");
        crossId = (String) DataUtils.parseRecord(payload, "crossId");
        appPayload = (Map<String, String>) DataUtils.parseRecord(payload, "appPayload");
    }

    public boolean isValid(){
        return isValidClientIdDeviceId() && isValidPayId() && isValidAppPayload();
    }

    private boolean isValidAppPayload() {
        if (MapUtils.isEmpty(appPayload)) {
            DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
                MetricName.INVALID_PAYLOAD);
            logger.logError("dropping records. invalid payload is coming.");
            return false;
          }
        return true;
    }

    private boolean isValidPayId() {
        if (payId == null) {
            DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
                MetricName.MISSING_PAY_ID);
            logger.logError("dropping records. invalid payId is coming.");
            return false;
          }
        return true;
    }

    private boolean isValidClientIdDeviceId() {
        if (clientId == null && deviceId == null) {
            DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
                MetricName.INVALID_ID);
            logger.logError("dropping records. invalid clientId and deviceId is coming.");
            return false;
          }
        return true;
    }

    public String getClientId() {
        return clientId;
    }

    public String getDeviceId() {
        return deviceId;
    }

    public String getPayId() {
        return payId;
    }

    public String getCrossId() {
        return crossId;
    }

    public Map<String, String> getAppPayload() {
        return appPayload;
    }
}
try {
  GenericRecordDomainDataDecoder decoder = new GenericRecordDomainDataDecoder(config);
  while (true) {
    ConsumerRecords<byte[], byte[]> records = consumer.poll(1000);
    for (ConsumerRecord<byte[], byte[]> record : records) {
      DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
          MetricName.TOTAL_NUMBER);
      GenericRecord payload = decoder.decode(record.value());

      //Replaced validation logic with the following 3 lines:
      PacketValidation validation = new PacketValidation(payload);
      if(!validation.isValid())
          continue;

      DataMetricHolder.getInstance().increment(validation.getPayId(), MetricName.TOTAL_NUMBER);

      Builder packetBuilder = new DataPacket.Builder(payload);
      if (validation.getClientId() != null) {
        packetBuilder.setClientId(validation.getClientId());
      }
      if (validation.getDeviceId() != null) {
        packetBuilder.setDeviceId(validation.getDeviceId());
      }
      if (validation.getCrossId() != null) {
        packetBuilder.setCrossId(validation.getCrossId());
      }
      DataPacket packet =
          packetBuilder.setPayId(validation.getPayId()).appPayload(validation.getAppPayload()).build();
      System.out.println(packet);
    }
  }
} catch (Exception ex) {
  // logging error
}
clientId = (String) DataUtils.parseRecord(payload, "clientId");
clientId = (String) DataUtils.parseRecord(payload, Constants.CLIENT_ID);
Map<String, String> appPayload...
if (MapUtils.isEmpty(appPayload))

Context

StackExchange Code Review Q#150163, answer score: 3

Revisions (0)

No revisions yet.