CODEWITHSUNDEEP
java
arsenal
arsenal

Reputation: 24154

Remove Repeated Code

My Question is-

I have two string variables site_inclusion and site_exclusion. If site_inclusion has a value, then I don't care what values site_exclusion contains. That is to say that site_inclusion overrides site_exclusion. If, however, site_inclusion is null and site_exclusion has a value, then I want to examine site_exclusion.

To be more precise:

  1. If site_inclusion and site_exclusion are both null then set useTheSynthesizer as true;
  2. If site_inclusion is not null and it matches with the regexPattern then set useTheSynthesizer as true. And I don't care what values are there in site_exclusion.
  3. If site_inclusion is null and site_exclusion is not null and site_exclusion does not match the regexPattern then set useTheSynthesizer to true.

I wrote the below code but somehow I think, I am repeating some stuff here in the if/else loop. Any code improvements will be appreciated that fulfill my conditions.

String site_inclusion = metadata.getSiteInclusion();
String site_exclusion = metadata.getSiteExclusion();

// fix for redundant data per site issue
if(site_inclusion != null && site_inclusion.matches(regexPattern)) {
    useTheSynthesizer = true;
} else if(site_exclusion != null && !(site_exclusion.matches(regexPattern))) {
    useTheSynthesizer = true;
} else if(site_inclusion == null && site_exclusion == null ) {
    useTheSynthesizer = true;
}

Upvotes: 3

Views: 152

Answers (5)

Edward Thomson
Edward Thomson

Reputation: 78703

  1. You don't really need the last null test.
  2. I (personally) find it poor style to do an if(test == true) flag = true statement. You can simply say flag = test.

My recommendation would be:

if(site_inclusion != null)
{
    useTheSynthesizer = site_inclusion.matches(regexPattern);
}
else if(site_exclusion != null)
{
    useTheSynthesizer = ! site_exclusion.matches(regexPattern);
}
else
{
    useTheSynthesizer = true;
}

You could also do it in a oneliner:

useTheSynthesizer = site_inclusion != null ? site_inclusion.matches(regexPattern) : (site_exclusion != null ? ! site_exclusion.matches(regexPattern) : true);

But I find that sort of obnoxious to read.

(Note, I made the assumption that useTheSynthesizer was otherwise false. This isn't explicit in your code or explanation, but I think this assumption was safe.)

Upvotes: 7

Manuel Cantonero
Manuel Cantonero

Reputation: 135

Hello I think you can improve it using some like OR, and no only AND or try somethin like swicht case.

Anyways you can create some function that tested your variables, and you can write this confusing code out from your main module.

For example you can write this code in a function called boolean TestingVariable ( String X, String Y);

For example: boolean TesteingVariable ( String X, String Y){

  if(X != null && X.matches(regexPattern)) {
      return true;
  } else if(Y != null && !(Y.matches(regexPattern))) {
      return = true;
  } else if(X == null && Y == null ) {
      return = true;
  }
};

On this way your final main module code will be something like this and you will avoid the confused code in your main code:

String site_inclusion = metadata.getSiteInclusion();
String site_exclusion = metadata.getSiteExclusion();

// fix for redundant data per site issue
useTheSynthesizer = TesteingVariable (site_inclusion ,site_exclusion);

I think you should enter the variable regexPattern in the function.

Sorry for my English I hope you can undertand all and it help to you.

Upvotes: 0

jahroy
jahroy

Reputation: 22692

I would do it like this:

    boolean useTheSynthesizer;

    if (siteInclusion == null && siteExclusion == null) {
        useTheSynthesizer = true;
    }
    else if (siteInclusion == null) {
        useTheSynthesizer = ( ! siteExclusion.matches(regexPattern) );
    }
    else {
        useTheSynthesizer = siteInclusion.matches(regexPattern);
    }

I also removed the underscores from your variable names, since they do not fit the java naming conventions (and they're hideous IMO).

Upvotes: 2

Naga
Naga

Reputation: 11

You can have 2 methods for flexibility to process the inclusions and exclusions like below.. 

callingMethod() {
  boolean useTheSynthesizer = processSiteInclusions(site_inclusion, regexPattern);

  if (useTheSynthesizer == false) {
     useTheSynthesizer = processSiteExclusions(site_inclusion, regexPattern);
  }

  if (useTheSynthesizer == false) {
    useTheSynthesizer = true;
  }
}

private boolean processSiteInclusions(site_inclusion, regexPattern) {
   boolean useSynthesizer = false;

   if (site_inclusion != null && !site_inclusion.matches(regexPattern))
      useSynthesizer = true;

   return useSynthesizer;
}

private boolean processSiteExclusions(site_exclusion, regexPattern) {
   boolean useSynthesizer = false;

   if (site_exclusion != null && !site_inclusion.matches(regexPattern))
      useSynthesizer = true;

   return useSynthesizer;
}

Upvotes: 0

raddykrish
raddykrish

Reputation: 1866

You can do like this. Basically i extracted all the conditions as small methods and made as OR condition.

    String site_inclusion = metadata.getSiteInclusion();
    String site_exclusion = metadata.getSiteExclusion();
        if(isInclusionAndExclusionNull(site_inclusion, site_exclusion) || isSiteExclusionMatches(site_exclusion, regexPattern) || isSiteInclusionMatches(site_inclusion, regexPattern)) {
            useTheSynthesizer = true;
        }

private static boolean isInclusionAndExclusionNull(String site_inclusion,
            String site_exclusion) {
        return site_inclusion == null && site_exclusion == null;
    }    
    private boolean isSiteExclusionMatches(String site_exclusion,
                String regexPattern) {
            return site_exclusion != null && !(site_exclusion.matches(regexPattern));
        }

        private  boolean isSiteInclusionMatches(String site_inclusion,
                String regexPattern) {
            return site_inclusion != null && site_inclusion.matches(regexPattern);
        }

Upvotes: 0

Related Questions