How to refactor a complicated expression in the if condition?

Question

Here's example:

if (folderInfoRecord.getValueRequired()
    && ((!folderInfoData.getInfoType().equals(InfoType.NUMERIC) 
    && !folderInfoData.getInfoType().equals(InfoType.DATE) 
    && !folderInfoData.getInfoType().equals(InfoType.CHOOSE) 
    && StringUtils.isBlank(folderInfoRecord.getInfoValue(), true))
    || (folderInfoData.getInfoType().equals(InfoType.NUMERIC) 
    && folderInfoRecord.getInfoValueNumeric() == null) 
    || (folderInfoData.getInfoType().equals(InfoType.DATE) 
    && folderInfoRecord.getInfoValueDateTime() == null) 
    || (folderInfoData.getInfoType().equals(InfoType.CHOOSE) 
    && folderInfoData.getSelectedInfoRole() == null))) {
        showNotification(pageResourceBundle.getText("JS_ALERT_FIELD_REQUIRED"));
        return;
}

Can anyone tell me how can optimize this if condition ?

Answer 1

The true solution is to Replace Conditional with Polymorphism. The biggest problem here is that a flag is encoding types.

---

I understand that it may need significant amount of work and you may not be in the position to do that. Here is a workaround that makes the intent of the code clearer and the code hopefully more readable:

if (folderInfoRecord.getValueRequired() && 
    Checker.isMissing(folderInfoData, folderInfoRecord) 
{
        showNotification(pageResourceBundle.getText("JS_ALERT_FIELD_REQUIRED"));
        return;
}

(The code below may not even compile, please check with your original one.)

final class Checker {

  InfoType         it;
  FolderInfoRecord fir;
  FolderInfoData   fid;

  private Checker(FolderInfoData fid, FolderInfoRecord fir) {
    this.it  = fid.getInfoType();
    this.fid = fid;
    this.fir = fir;
  }

  static public boolean isMissing(FolderInfoData fid, FolderInfoRecord fir) {

      return (new Checker(fid, fir)).isMissing();
  }

  private boolean isMissing() {

     return wrongAsNumeric() && wrongAsDate() && wrongAsChoose() && isBlank();
  }

  private boolean wrongAsNumeric() {

    return it.equals(InfoType.NUMERIC) && (fir.getInfoValueNumeric() == null);
  }

  private boolean wrongAsDate() {

    return it.equals(InfoType.DATE) && (fir.getInfoValueNumeric() == null);
  }

  private boolean wrongAsChoose() {

    return it.equals(InfoType.CHOOSE) && folderInfoData.getSelectedInfoRole() == null)
  }

  private boolean isBlank() {

    return StringUtils.isBlank(folderInfoRecord.getInfoValue(), true);
  }  
}

Unlike the other proposed solutions, this code is reusable: If you want to perform this check elsewhere in the code, you simply call Checker.isMissing().

Answer 2

Well, if I am reading this right this would reduce to no more than 4 function calls to determine if the alert needs to be raised. So 1 less at least... a little faster.

Oh yea also readable.

UPDATE:

folderInfoData.getInfoType() need not be evaluated for all 3 types unconditionally, modified to reflect that. Still readable.

if(folderInfoRecord.getValueRequired())
{
   Boolean alertNeeded = false;

   if(folderInfoData.getInfoType().equals(InfoType.NUMERIC)) 
   {
      if(folderInfoRecord.getInfoValueNumeric() == null) alertNeeded = true;
   }
   else if(folderInfoData.getInfoType().equals(InfoType.DATE))
   {  
      if(folderInfoRecord.getInfoValueDateTime() == null) alertNeeded = true;
   }
   else if(folderInfoData.getInfoType().equals(InfoType.CHOOSE))
   {
      if(folderInfoData.getSelectedInfoRole() == null) alertNeeded = true;
   }
   else
   {
      if(StringUtils.isBlank(folderInfoRecord.getInfoValue(), true))
         alertNeeded = true;
   }

   if(alertNeeded) 
   { 
      showNotification(pageResourceBundle.getText("JS_ALERT_FIELD_REQUIRED"));
      return;
   }
}

Answer 3

You can also split the if logic here to make it more readable. Alternatively I would suggest you to create a function that will do all these checks and return true of false. That way you don't have to put all the checks at one location. You aren't going to get any performance advantage by doing either approaches I suggested, but it will be more readable.

Approach1:

infoType = folderInfoData.getInfoType();

if (folderInfoRecord.getValueRequired())
{
    if ((!infoType.equals(InfoType.NUMERIC) && !infoType.equals(InfoType.DATE) && !infoType.equals(InfoType.CHOOSE) && StringUtils.isBlank(folderInfoRecord.getInfoValue(), true)))
    {
       showNotification(pageResourceBundle.getText("JS_ALERT_FIELD_REQUIRED"));
       return;
    }
    if((infoType.equals(InfoType.NUMERIC) && folderInfoRecord.getInfoValueNumeric() == null))
    {
       showNotification(pageResourceBundle.getText("JS_ALERT_FIELD_REQUIRED"));
       return;
    }
    if(infoType.equals(InfoType.DATE) && folderInfoRecord.getInfoValueDateTime() == null)
    {
       showNotification(pageResourceBundle.getText("JS_ALERT_FIELD_REQUIRED"));
       return;
    }
    if(infoType.equals(InfoType.CHOOSE) && folderInfoData.getSelectedInfoRole() == null)
    {
       showNotification(pageResourceBundle.getText("JS_ALERT_FIELD_REQUIRED"));
       return;
    }
}

Approach 2:

if(checkConditions(folderInfoData,folderInfoRecord) == true)
{
     showNotification(pageResourceBundle.getText("JS_ALERT_FIELD_REQUIRED"));
     return;
}


public bool checkConditions(Object folderInfoData, Object folderInfoRecord)
{
     infoType = folderInfoData.getInfoType();

    if (folderInfoRecord.getValueRequired())
    {
        if ((!infoType.equals(InfoType.NUMERIC) && !infoType.equals(InfoType.DATE) && !infoType.equals(InfoType.CHOOSE) && StringUtils.isBlank(folderInfoRecord.getInfoValue(), true)))
        {
           return true;
        }
        if((infoType.equals(InfoType.NUMERIC) && folderInfoRecord.getInfoValueNumeric() == null))
        {
           return true;
        }
        if(infoType.equals(InfoType.DATE) && folderInfoRecord.getInfoValueDateTime() == null)
        {
           return true;
        }
        if(infoType.equals(InfoType.CHOOSE) && folderInfoData.getSelectedInfoRole() == null)
        {
           return true;
        }
    }
    return false;
}

You can further optimize it by doing infoType.equals checks before starting the if branch and store them in boolean logic to further simplify it. The example of that is already given in other answers. By clubbing this approach with other approaches the code will be better in my view. Also keep in mind that the Object type must be changed to the type of your objects for this code to work. I just gave it as example as I don't have any idea of what is the type of your objects.

Answer 4

final String info = folderInfoData.getInfoType();
final boolean isNumeric = info.equals(InfoType.NUMERIC);
final boolean isDate = info.equals(InfoType.DATE);
final boolean isChoose = info.equals(InfoType.CHOOSE);
final boolean isBlank = StringUtils.isBlank(folderInfoRecord.getInfoValue(), true);

if (folderInfoRecord.getValueRequired() && 
    ((!isNumeric && !isDate && !isChoose && isBlank)
    || (isNumeric && folderInfoRecord.getInfoValueNumeric() == null) 
    || (isDate && folderInfoRecord.getInfoValueDateTime() == null) 
    || (isChoose && folderInfoData.getSelectedInfoRole() == null))) {

    showNotification(pageResourceBundle.getText("JS_ALERT_FIELD_REQUIRED"));
    return;
}

Answer 5

By optimize, if you mean run-time complexity, there's not much I can say because I haven't the foggiest what each method does. I would strongly advise against spaghetti code like the one you're showing. For the confusing NOTs and ORs I would avoid having the .equals or the .isBlank's be computed inside the if statement. Have some external boolean flags outside like

bool folderDataisNumeric = folderInfoData.getInfoType().equals(InfoType.NUMERIC) 

and then simply using the flag !folderDataisNumeric will make the code more humanly readable.

My two cents, anyway.