0
\$\begingroup\$

I have code where I validate deviceId and clientId basis on what ProcessType is coming:

  public static boolean isValid(ProcessType processType, Record record) {
    String deviceId = DataUtils.extract(record, "deviceId");
    String clientId =DataUtils.extract(record, "clientId");

    if(processType == ProcessType.ABC || processType == ProcessType.DEF) {
        if (Strings.isNullOrEmpty(clientId) || Strings.isNullOrEmpty(deviceId)) {
        LogMetric.getInstance().increment(processType.name(), Metric.ID);
        logger.logError("invalid clientId and deviceId provided. ProcessType= ", processType,
            ", Record= ", record);          
          return false;
        }
    } else {
        if (Strings.isNullOrEmpty(clientId) && Strings.isNullOrEmpty(deviceId)) {
        LogMetric.getInstance().increment(processType.name(), Metric.ID);
        logger.logError("invalid clientId and deviceId provided. ProcessType= ", processType,
            ", Record= ", record);          
          return false;
        }   
    }
    return true;
  }
  • If ProcessType is coming as ABC or DEF, then I will return false if clientId or deviceId is missing
  • Otherwise I will return false if both clientId and deviceId are missing.
  • If all the conditions are not met, then I will return true.

I wanted to see if there is any better way to write this. I'm working with Java 7.

\$\endgroup\$
1
\$\begingroup\$

Except the wrong indentation in your deepest if there is nothing much to do. But :

You can create a isSpecificProcess(ProcessType):boolean method that contains the processType == ProcessType.ABC || processType == ProcessType.DEF condition. Il will explain the meaning of your test for a newcomer in your business.

You can also extract the logging to a specific method because the two blocks are identical.

Aside of that I can't see anything more to say.

\$\endgroup\$
2
\$\begingroup\$

I was not able to tell what your isValid method did upon first reading. I therefore renamed the method to be more explicit about what the method is validating and changed the parameter order to reflect the new name.

I abstracted out the Logging and ID extraction into their own methods to make the code more readable and follow the single responsibility principle. Now, each method works on the same level of abstraction and has a defined role.

I abstracted out the actual record validation checks ( i.e. isRecordValidForABCOrDEFProcessType ) to encapsulate the checking logic and also make the checks readable. The longer method names make the code self documenting.

Finally, I gave the boolean expressions names as the expressions are the most vital logic of the record validation. The logic will also be reread often, so you want the logic to be as clear as possible.

  public static boolean isRecordValidForProcessType( Record record, ProcessType processType ) {
    boolean isProcessTypeABCOrDEF = processType == ProcessType.ABC || processType == ProcessType.DEF;
    boolean isRecordValid = isProcessTypeABCOrDEF 
      ? isRecordValidForABCOrDEFProcessType( record ) 
      : isRecordValidForNonABCOrDEFProcessType( record );

    if( isRecordValid == false )
        logInvalidRecordErrorForProcessType( record, processType ); 

    return isRecordValid;
  }

  private static boolean isRecordValidForABCOrDEFProcessType(Record record){
    String deviceId = getDeviceIdFromRecord(record);
    String clientId = getClientIdFromRecord(record);
    boolean doesRecordHaveValidIds = ( Strings.isNullOrEmpty(clientId) || Strings.isNullOrEmpty(deviceId) ) == false;
    return doesRecordHaveValidIds;
  }

  private static boolean isRecordValidForNonABCOrDEFProcessType(Record record) {
    String deviceId = getDeviceIdFromRecord(record);
    String clientId = getClientIdFromRecord(record);
    boolean doesRecordHaveValidIds = ( Strings.isNullOrEmpty(clientId) && Strings.isNullOrEmpty(deviceId) ) == false;
    return doesRecordHaveValidIds;
  }


  private static String getDeviceIdFromRecord(Record record) {
    return DataUtils.extract(record, "deviceId");
  }

  private static String getClientIdFromRecord(Record record) {
    return DataUtils.extract(record,"clientid");
  }

  private static void logInvalidRecordErrorForProcessType(Record record, Process processType){
    LogMetric.getInstance().increment(processType.name(), Metric.ID);
      logger.logError("invalid clientId and deviceId provided. ProcessType= ", processType,
                      ", Record= ", record);
  }
\$\endgroup\$

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.