Optimizing nested if/ then, java
This code's only redeeming quality is that it works. Can you please help me structure it better?
if (profile.isIgnoreCase()) {
// ignore case
if (masterKey.equalsIgnoreCase((targetKey))) {
if (masterValue.equalsIgnoreCase(targetValue)) {
doOK(masterKey, masterValue);
break;
} else {
// Key is either Missing or is an Error
if (checkErrors) {
doError(masterKey, masterValue, targetValue);
break;
开发者_JAVA百科 }
}
}
} else {
if (masterKey.equals(targetKey)) {
if (masterValue.equals(targetValue)) {
doOK(masterKey, masterValue);
break;
} else {
if (checkErrors) {
doError(masterKey, masterValue, targetValue);
break;
}
}
}
}
You can remove some of the repetition by using:
if (profile.isIgnoreCase()) {
masterKey = masterKey.toLowerCase();
masterValue = masterValue.toLowerCase();
}
if (masterKey.equals(targetKey)) {
if (masterValue.equals(targetValue)) {
doOK(masterKey, masterValue);
} else {
if (checkErrors) {
doError(masterKey, masterValue, targetValue);
}
}
}
I have also removed the break
s as I doesn't look like you need them to me
[UPDATE] Alternatively, how about writing a new method to handle the comparison
public boolean isEqual(String a, String b, boolean ignoreCase) {
if (ignoreCase) {
return a.equalsIgnoreCase(b);
} else {
return a.equals(b);
}
}
you would then update your code like so:
if (isEqual(masterKey,targetKey,profile.isIgnoreCase())) {
if (isEqual(masterValue,targetValue,profile.isIgnoreCase())) {
doOK(masterKey, masterValue);
} else {
if (checkErrors) {
doError(masterKey, masterValue, targetValue);
}
}
}
Pull out the key and value comparisons into local variables and you can eliminate the duplicated logic. This avoids modifying the strings, and as a bonus makes the if
statements a bit easier on the eyes.
boolean keysMatch, valuesMatch;
if (profile.isIgnoreCase()) {
keysMatch = masterKey .equalsIgnoreCase(targetKey);
valuesMatch = masterValue.equalsIgnoreCase(targetValue);
} else {
keysMatch = masterKey .equals(targetKey);
valuesMatch = masterValue.equals(targetValue);
}
if (keysMatch) {
if (valuesMatch) {
doOK(masterKey, masterValue);
break;
} else {
// Key is either Missing or is an Error
if (checkErrors) {
doError(masterKey, masterValue, targetValue);
break;
}
}
}
You could make it shorter by replacing them with the booleans that they represent, ie: masterKey.equalsIgnoreCase((targetKey))
with true
or false
.Which could help make it shorter, because you would need one less if
and else
clause.
bool ignoreCase = profile.isIgnoreCase();
if ((ignoreCase && masterKey.equalsIgnoreCase(targetKey)) ||
(!ignoreCase && masterKey.equals(targetKey))){
if ((ignoreCase && masterValue.equalsIgnoreCase(targetValue)) ||
(!ignoreCase && masterValue.equals(targetValue))) {
doOK(masterKey, masterValue);
break;
} else
if (checkErrors) {
doError(masterKey, masterValue, targetValue);
break;
}
}
Should be quicker this way to read boolean value, if this evaluates to false the program can skip to the next evaluation sooner than running the string compare first.
don't know if you need the break there. But I believe this is completely equivalent to your code.
bool ignoreCase = profile.isIgnoreCase();
if(ignoreCase and masterKey.equalsIgnoreCase(targetKey) or !ignoreCase and masterKey.equals(targetKey)) {
if(ignoreCase and masterValue.equalsIgnoreCase(targetValue) or !ignoreCase and masterValue.equals(targetValue)) {
doOK(masterKey, masterValue);
break;
} else if(checkErrors) {
doError(masterKey, masterValue, targetValue);
break;
}
}
Or I would write the following function in masterKey and masterValue class
public bool equalsCheckCase(targetKey, ignoreCase) {
if(ignoreCase) {
return this.equalsIgnoreCase(targetKey)
} else {
return this.equals(targetKey);
}
}
So the code sample becomes more readable.
bool ignoreCase = profile.isIgnoreCase();
if(masterKey.equalsCheckCase(targetKey, ignoreCase)) {
if(masterValue.equalsCheckCase(targetValue, ignoreCase)) {
doOK(masterKey, masterValue);
break;
} else if(checkErrors) {
doError(masterKey, masterValue, targetValue);
break;
}
}
精彩评论