How to validate each process object from its own validator?

Question

I have two process and for each process, I will get different Record object and I need to validate those Record object. This means I cannot use single validator as I have to validate different fields for both the process.

• For processA, I am using ValidatorA class to validate its Record object.
• For processB, I am using ValidatorB class to validate its Record object.

If they are valid, then I will move forward otherwise I won't move forward. Below is my process code both for A and B.

public class ProcessConsumer implements Runnable {
  private static final Logger logger = Logger.getInstance(ProcessConsumer.class);
  private final String processName;
  private final Validator validator;
  private final RecordProcessor<byte[], byte[]> process;

  public ProcessConsumer(String processName, Validator validator) {
    this.processName = processName;
    this.validator = validator;
    this.process = new RecordProcessor<>();
  }

  @Override
  public void run() {
    try {
      process.subscribe(getTopicsBasedOnProcessName(processName));
      ....

      while (true) {
        ConsumerRecords<byte[], byte[]> crs = process.poll(2000);
        for (ConsumerRecord<byte[], byte[]> cr : crs) {
          // record object will be different for my both the processes.
          Record record = decoder.decode(cr.value());
          Optional<DataHolder> validatedHolder = validator.getDataHolder(processName, record);
          if (!validatedHolder.isPresent()) {
            logger.logError("records dropped. payload= ", record);
            continue;
          }
          // send validatedHolder to processor class
          Processor.getInstance().execute(validatedHolder);
        }
      }
    } catch (Exception ex) {
      logger.logError("error= ", ExceptionUtils.getStackTrace(ex));
    }
  }
}

Below is my ValidatorA class in which I am validating few fields on record object and if it is valid, then I am returning DataHolder.

public class ValidatorA extends Validator {
  private static final Logger logger = Logger.getInstance(ValidatorA.class);

  @Override
  public static Optional<DataHolder> getDataHolder(String processName, Record record) {
    Optional<DataHolder> dataHolder = Optional.absent();
    if (isValid(processName, record))
      dataHolder = Optional.of(buildDataHolder(processName, record));
    return dataHolder;
  }

  private DataHolder isValid(String processName, Record record) {
    return isValidClientIdDeviceId(processName, record) && isValidPayId(processName, record)
        && isValidHolder(processName, record)
  }  

  private DataHolder buildDataHolder(String processName, Record record) {
    Map<String, String> holder = (Map<String, String>) DataUtils.extract(record, "holder");
    String deviceId = (String) DataUtils.extract(record, "deviceId");
    Integer payId = (Integer) DataUtils.extract(record, "payId");
    String clientId = (String) DataUtils.extract(record, "clientId");

    // add mandatory fields in the holder map after getting other fields
    holder.put("isClientId", (clientId == null) ? "false" : "true");
    holder.put("isDeviceId", (clientId == null) ? "true" : "false");
    holder.put("abc", (clientId == null) ? deviceId : clientId);

    return new DataHolder.Builder(record).setClientId(clientId).setDeviceId(deviceId)
        .setPayId(String.valueOf(payId)).setHolder(holder).build();
  }

  private boolean isValidHolder(String processName, Record record) {
    Map<String, String> holder = (Map<String, String>) DataUtils.extract(record, "holder");
    if (MapUtils.isEmpty(holder)) {
      logger.logError("invalid holder is coming.");
      return false;
    }
    return true;
  }

  private boolean isValidpayId(String processName, Record record) {
    Integer payId = (Integer) DataUtils.extract(record, "payId");
    if (payId == null) {
      logger.logError("invalid payId is coming.");
      return false;
    }
    return true;
  }

  private boolean isValidClientIdDeviceId(String processName, Record record) {
    String deviceId = (String) DataUtils.extract(record, "deviceId");
    String clientId = (String) DataUtils.extract(record, "clientId");
    if (Strings.isNullOrEmpty(clientId) && Strings.isNullOrEmpty(deviceId)) {
      logger.logError("invalid clientId and deviceId is coming.");
      return false;
    }
    return true;
  }
}

And below is my ValidatorB class in which I am validating few different fields as compared to ValidatorA on record object and if it is valid, then I am returning DataHolder.

public class ValidatorB extends Validator {
  private static final Logger logger = Logger.getInstance(ValidatorB.class);

  @Override
  public static Optional<DataHolder> getDataHolder(String processName, Record record) {
    Optional<DataHolder> dataHolder = Optional.absent();
    if (isValid(processName, record))
      dataHolder = Optional.of(buildDataHolder(processName, record));
    return dataHolder;
  }

  private DataHolder isValid(String processName, Record record) {
    return isValidType(processName, record) && isValidDatumId(processName, record) && isValidItemId(processName, record);
  }  

  private DataHolder buildDataHolder(String processName, Record record) {
    String type = (String) DataUtils.extract(record, "type");
    String datumId = (String) DataUtils.extract(record, "datumId");
    String itemId = (String) DataUtils.extract(record, "itemId");

    return new DataHolder.Builder(record).setType(type).setDatumId(datumId)
        .setItemId(itemId).build();
  }


  private boolean isValidType(String processName, Record record) {
    String type = (String) DataUtils.extract(record, "type");
    if (Strings.isNullOrEmpty(type)) {
      logger.logError("invalid type is coming.");
      return false;
    }
    return true;
  }  

  private boolean isValidDatumId(String processName, Record record) {
    String datumId = (String) DataUtils.extract(record, "datumId");
    if (Strings.isNullOrEmpty(datumId)) {
      logger.logError("invalid datumId is coming.");
      return false;
    }
    return true;
  }   

  private boolean isValidItemId(String processName, Record record) {
    String itemId = (String) DataUtils.extract(record, "itemId");
    if (Strings.isNullOrEmpty(itemId)) {
      logger.logError("invalid itemId is coming.");
      return false;
    }
    return true;
  }
}

And below is my abstract class:

public abstract class Validator {
  public abstract Optional<DataHolder> getDataHolder(String processName, Record record);
}

Question:

This is how I am calling for both of my process. As you can see, I am passing processName and its particular validator in the constructor argumnets.

ProcessConsumer processA = new ProcessConsumer("processA", new ValidatorA());
ProcessConsumer processB = new ProcessConsumer("processB", new ValidatorB());

• Is this a good design where for each of my process, pass its validator along with?
• Is there any way we can avoid passing that? And internally figure out what validators to use basis on the processName? I already have an enum with all my processName. I need to make this design extensible so that if I add new process in future, it should be scalable.
• Also the way I have my abstract class Validator is right? It is not doing any useful things at all looks like.

Each of my Validator is basically trying to validate whether the record object is valid or not. If they are valid, then they make DataHolder builder and return it, otherwise it returns Optional.absent();

I saw this post where they talked about using Decorator pattern but I am not sure how that will help me in this case.

Answer 1

First when I see the declaration and the implementation of them :

public abstract class Validator {
  public abstract Optional<DataHolder> getDataHolder(String processName, Record record);
}

I don't think "Validator" is the best term. Your validators are not only validators. What you call validators have as main function : extract data for a specific process. The extraction requires a validation but it is not the main function.
While the main function of a validator is validating.
So I think you could call them something as : ProcessDataExtractor.

ProcessConsumer processA = new ProcessConsumer("processA", new ValidatorA());
ProcessConsumer processB = new ProcessConsumer("processB", new ValidatorB());

Is this a good design where for each of my process, pass its validator along with? Is there any way we can avoid passing that? And internally figure out what validators to use basis on the processName? I already have an enum with all my processName. I need to make this design extensible so that if I add new process in future, it should be scalable.

Scalability is another thing.
Having a extensible design is broadly having a design which doesn't imply important and or risky modifications as soon as a new "normal" requirement happens in the life of the application.
If a new process consumer is added, you have to add a ProcessDataExtractor according to your needs. The client should be aware of this new process consumer.
If the client code instantiate its process consumer and its data extractor at compile-time, using enum and map to represent process consumers and data extractors doesn't make your design not expandable since it requires very little of modification and these are isolated

If you want to have still less of modification in your code, you could instantiate by reflection the extractor and using a naming convention to retrieve them.
For example, place them always in the same package and name each extractor with the same prefix, for example : ProcessDataExtractorXXX or XXX is the variable part.
The problem of this solution is at compile time : clients doesn't know necessary the ProcessDataExtractor available.

If you want that the adding of a new process consumer and extractor to be dynamic, that is during the runtime of the application and that clients may retrieve them during runtime too, it is another subject I think.

At compile-time, the design could be better because so far the client of the ProcessConsumer and ProcessDataExtractor classes may make a bad use of them (that is using Process A with ProcessDataExtractor B).
To avoid that, you have several ways of doing.
But you have guessed the idea : making the initialization and the mapping between ProcessConsumer and ProcessDataExtractor in a dedicated place and a protected way.

To achieve it, I advise you to introduce a interface for ProcessConsumer which provides only the functional method from Runnable:

public interface IProcessConsumer extends Runnable {

}

From now clients who want to consume a process should only use this interface to perform their task.
We don't want provide to the client method or constructor to choose its data extractor.
To do it, the concrete ProcessConsumer class should be an inner private class in order to not allow clients to instantiate it directly.
They will have to use a factory to address this need.
In this way, client are able to create the specific process consumer with the required data extractor by requesting a factory of Processes which is responsible to ensure the consistence between a data extractor and a process and which also guarantees the instantiation of a new process consumer at each call (your processes are stateful, so you have to create a new process consumer for each process consumer you start).

Here is the ProcessConsumerFactory class :

import java.util.HashMap;
import java.util.Map;

public class ProcessConsumerFactory {

    public static enum ProcessType {

      A("processA"), B("processB");

      private String name;

      ProcessType(String name) {
        this.name = name;
      }

      public String getName() {
        return name;
      }
    }

    private class ProcessConsumer implements IProcessConsumer {

      private final ProcessType processType;
      private final ProcessDataExtractor extractor;
      private final RecordProcessor<byte[], byte[]> process;

      public ProcessConsumer(ProcessType processType, ProcessDataExtractor extractor) {
        this.processType = processType;
        this.extractor = extractor;
        this.process = new RecordProcessor<>();
      }

      @Override
      public void run() {
         // your implementation...
      }
    }

    private static ProcessConsumerFactory instance = new ProcessConsumerFactory();

    private Map<ProcessType, ProcessDataExtractor> extractorsByProcessName;

    private ProcessConsumerFactory() {
      extractorsByProcessName = new HashMap<>();
      extractorsByProcessName.put(ProcessType.A, new ProcessDataExtractorA());
      extractorsByProcessName.put(ProcessType.B, new ProcessDataExtractorB());
      // add a new element in the map to add a new mapping
    }

    public static ProcessConsumerFactory getInstance() {
      return instance;
    }

    public IProcessConsumer createNewProcessConsumer(ProcessType  processType) {
      ProcessDataExtractor extractor = extractorsByProcessName.get(processType);
      if (extractor == null) {
        throw new IllegalArgumentException("processType " + processType + " not recognized");
      }
      IProcessConsumer processConsumer = new ProcessConsumer(processType, extractor);
      return processConsumer;
    }

}

Now the clients of the Process consumers class could instante them like that:

IProcessConsumer processConsumer = ProcessConsumerFactory.getInstance().createNewProcessConsumer(ProcessType.A);

Also the way I have my abstract class Validator is right? It is not doing any useful things at all looks like.

You use an abstract class for validators but for the moment you have not move common behavior in this abstract class, so you could use an interface :

public interface ProcessDataExtractor{
  Optional<DataHolder> getDataHolder(ProcessType processType, Record record);
}

You could introduce the abstract class later if it becomes suitable.

Answer 2

There are a few problems with your design:

Catch invalid data as early as possible.

Post-construction is not the right way. Once an object, in this case Record, is constructed it should have valid state. Which means, your validation should be performed prior to constructing Record.

1. Get data from somehere: from web, database, text file, user input or whatever.
2. Validate data.
3. Construct Record object. At this point, either Record object has valid state, or it fails construction for example by raising exception.

Now, if the source from which you get data, contains mostly invalid data, it should be dealt there. Because that is a problem in itself. If the source is right but reading or getting the data has problems, it should be solved first.

Assuming the above issues, if exists, solved then the sequence of program should be something like this.

// Get the data from some source
// Perform Validation on the data. This is generic validation, like validation // of data read from an input form etc.
validate deviceId
validate payId 
validate clientId 
...
if invalid do something
else if valid proceed

// construct Record object
Record record = new Record(deviceId, payId, clientId, ...)
// At this point record has valid data

public class Record {

 deviceId
 payId 
 clientId 
 Record(deviceId, payId, clientId, ...) {
    // Perform business rule validation, pertaining to Record's requirements.
    // For example, if deviceId must be in certain range etc.

    // if data is valid, perform construction.
    // else if invalid, don't construct. throw exception or something 
    // to deal with invalid condition
 }
}

Another problem is, you use some utils class to extract internal data from Record. That is not right either. Record itself should provide getters to its attributes. Right now, what is related to Record is scattered between Record, Utils, and Validator.

I think your code needs a thorough re-evaluation. I suggest, take a pause, start again but this time at a higher level. Start designing without code for example with some sort of diagramming. Start with only box and arrows (Something like a class diagram but don't need to use a UML tool etc. Pencil and paper. Decide what should go where. Things like,

1. what are the entities you are dealing with.
2. What properties each entity has, attributes, methods etc.
3. What is the relationship among these entities
4. Consider the sequence these entities are used or interact, then keep refining it.

Without this high level view, dealing with the design question at the code level is difficult and almost always gives bad results.

Once you dealt with the design at a higher level. Then, putting that in code is much easier. Of course you can refine it at that level too, but high level structure should be considered first.