Inclusion and exclusion

Question

Problem Statement:

First Case: Where inclusion and exclusion both are not empty so that means inclusion overrides anything in exclusion so for that algorithm is like this:

If key is either equal to 155 or 156 and if temp is there in inclusion then go into that if loop and do whatever you want. But if temp is not there in inclusion then don't do anything.

Second Case: Where inclusion is null and exclusion is not null so that means now you need to exclude everything present in exclusion string, so the algorithm is like this:

If key is either equal to 155 or 156 and if temp is not there in exclusion then do whatever you want to do in that else loop otherwise don't do anything.

Third Case: If key is not equal to 155 or 156 then do whatever you want to do in that loop.

Is there any way we can improve this code in terms of performance, readability, accuracy? as when I see this it contains lot of if/else statements to make the above problem works

P.S: Things that I am doing in each if/else loop is same everytime.

Cases1: Where input parameters can be-

String inclusion = null; // It can be null also
String exclusion = "100;77";

String temp = "0"; // It can change anytime

Cases2: Where input parameter can be-

String inclusion = "0"; // It can be null also
String exclusion = null;

String temp = "0"; // It can change anytime

Below is the code in which above input parameters can be used:

for(TesxAttrKey key: pdsxWriteRequest.getAttrKeys()){

if((key.equals("156") || key.equals("155"))) {
    if(inclusion != null && !inclusion.trim().isEmpty()) {
        if(inclusion.matches("(^|.*;)" + temp + "(;.*|$)")) {
            //Check the DsxAttrValue has a Synthesizer
            if(metadata!=null){
                Synthesizer synthesizer = metadata.getSynthesizer();
                if(synthesizer!=null){
                    //Synthesize the attribute
                    DsxAttrValue synthesizedVal = 
synthesizer.synthesize(key, record);
                    val = synthesizedVal;
                }
            }
            if(val!=null){
                writeRecord.getAttrs().put(key, val);
            }        
        }
    } else {
        if(!exclusion.matches("(^|.*;)" + temp + "(;.*|$)")) {
            //Check the DsxAttrValue has a Synthesizer
            if(metadata!=null){
                Synthesizer synthesizer = metadata.getSynthesizer();
                if(synthesizer!=null){
                    //Synthesize the attribute
                    DsxAttrValue synthesizedVal = 
synthesizer.synthesize(key, record);
                    //if(synthesizedVal!=null){
                    val = synthesizedVal;
                    //}
                }
            }
            if(val!=null){
                writeRecord.getAttrs().put(key, val);
            }
        }
    }
} else {
    //Check the DsxAttrValue has a Synthesizer
    if(metadata!=null){
        Synthesizer synthesizer = metadata.getSynthesizer();
        if(synthesizer!=null){
            //Synthesize the attribute
            DsxAttrValue synthesizedVal = synthesizer.synthesize(key, record);
            //if(synthesizedVal!=null){
            val = synthesizedVal;
            //}
        }
    }
    if(val!=null){
        writeRecord.getAttrs().put(key, val);
    }

}

Is there any way we can improve this code in terms of performance, as I have lot of if/else loops to solve the above problem?

Answer 1

Your logic seems good. The problem statement was a tough to understand, but I think I got it.

Since inclusion and exclusion do not change, we should extract that checking from the loop and place it before the loop.

Furthermore, to reduce programmer errors, I reduced the code duplicity. The "do whatever you want" is now only typed once, and the regex pattern is only typed once.

String tempPattern = "(^|.*;)" + temp + "(;.*|$)";

boolean tempInInclusionOrNotInExclusion;
if(inclusion != null && !inclusion.trim().isEmpty()){
    tempInInclusionOrNotInExclusion = inclusion.matches(tempPattern);
} else if(exclusion != null) {
    tempInInclusionOrNotInExclusion = !exclusion.matches(tempPattern);
} else {
    tempInInclusionOrNotInExclusion = true;
}

for(PdsxAttrKey key: pdsxWriteRequest.getAttrKeys()){
    boolean shouldDoWhatIWantToDo;
    if(key.equals("156") || key.equals("155")) {
        shouldDoWhatIWantToDo = tempInInclusionOrNotInExclusion;
    } else {
        shouldDoWhatIWantToDo = true;
    }

    if(shouldDoWhatIWantToDo){
        //Check the PdsxAttrValue has a Synthesizer
        if(metadata!=null){
            Synthesizer synthesizer = metadata.getSynthesizer();
            if(synthesizer!=null){
                //Synthesize the attribute
                PdsxAttrValue synthesizedVal = synthesizer.synthesize(key, record);
                //if(synthesizedVal!=null){
                val = synthesizedVal;
                //}
            }
        }
        if(val!=null){
            writeRecord.getAttrs().put(key, val);
        }
    }
}

Answer 2

I would always recommend util classes for common checks like:

if(inclusion != null && !inclusion.trim().isEmpty()) {

--> StringUtil.isNotBlank(inclusion)

This makes code more readable. If you don't have these kind of util classes from former projects just use public implementations (from Spring, for example).

Answer 3

Regarding the performance:

Are you sure there are any actual performance problems in that code? My point is that it could actually make the code slower when you try to improve code that you "think" is slow.

Here's an answer I gave recently to a related question. Its for a different programming language and a different subject matter, but the concept is the same. If you make changes to the code, how will you know if it is faster?

Something else to consider: What are the performance requirements? And remember a requirement that states "As fast as possible..." is not a valid requirement.