ansaurus

Question

Answer 1

A:

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.

TaslemGuy 2010-09-20 21:41:01

Moving the expressions into separate boolean variables (if that's what you mean) would only make sense if some of the if expressions were repeating, which they're not.

EboMike 2010-09-20 21:51:39

That's not true! He has two identical if-then structures simply nested into in if/else structure! That's not efficient: he could simply process the if/else BEFOREHAND and THEN run the nested loop.

TaslemGuy 2010-09-21 21:11:23

Answer 2

+2 A:

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 breaks 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);
        }
    }
}

irishbuzz 2010-09-20 21:42:45

You need to ensure that targetKey/targetValue are lower-case as well, of course.

EboMike 2010-09-20 21:44:37

bah of course - well spotted EboMike

irishbuzz 2010-09-20 21:46:42

Good idea, but we are changing the data here. MaSteR key may be mixed case, which, becomes relevant later on in the program. Maybe temp variables .. for comparison purposes ... hmm ..

mac 2010-09-20 21:52:26

The only thing I'd say here is that if doError expects the masterKey and masterValue with case intact you have a problem.

Prescott 2010-09-20 21:52:31

@mac/Prescott good points. I've updated my original answer with another suggestion that looks more suited to the requirements. The isEqual if conditions are a little ugly but

irishbuzz 2010-09-20 22:07:13

Loving it, Thank you so much

mac 2010-09-20 22:14:40

Answer 3

A:

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;
    }
}

ontrack 2010-09-20 22:03:10

Answer 4

+1 A:

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;
        }
    }
}

John Kugelman 2010-09-20 22:10:22

Answer 5

A:

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.

spdub 2010-09-20 22:29:56

ansaurus

tags:

views:

answers:

Optimizing nested if/ then, java

related questions