CODEWITHSUNDEEP
javaloops
Alexander Walker
Alexander Walker

Reputation: 65

Is there a better way of writing this loop?

I need to check whether at least 1 item in a list has X, Y, and Z (not all at the same time). e.g. item 1 has x, and item 2 has y and z.

I thought it'd be better to do this without creating multiple loops and just checking for one of them, but instead store a variable and then check it so it can't be set to false again once true.

Seems like I'm probably missing a better way to do this, so is there one?

Thanks

        boolean hasX = false;
        boolean hasY = false;
        boolean hasZ = false;

        for (ItemType item : Items) {
            if (!hasX) { hasX = DoesHaveX(item); }
            if (!hasY) { hasY = DoesHaveY(item); }
            if (!hasZ) { hasZ = DoesHaveZ(item); }
    }

Upvotes: 0

Views: 161

Answers (4)

BeUndead
BeUndead

Reputation: 3618

Just to add a BitSet variant too, and under the assumption that checking has... is a semi-expensive operation:

private static final int xBit = 0;
private static final int yBit = 1;
private static final int zBit = 2;

public static boolean hasAll(final Collection<ItemType> items) {
    if (items.isEmpty()) return false;

    final BitSet bits = new BitSet(3);

    for (final ItemType item : items) {

        // Check if bit is already set to avoid
        // needless `has*` evaluation
        if (!bits.get(xBit) && hasX(item)) bits.set(xBit);

        if (!bits.get(yBit) && hasY(item)) bits.set(yBit);

        if (!bits.get(zBit) && hasZ(item)) bits.set(zBit);

        // You could repeat this INSIDE all of the 'if's
        // above to potentially avoid computing bits.get
        // but I'd sacrifice that for the slightly improved
        // readability.
        if (bits.cardinality() == 3) return true;
    }

    return false;
}

I can't tell you if this is faster or anything, as that depends on your has* implementations, amongst other things. But it avoids most recomputations whereever possible.

Upvotes: 0

Bentaye
Bentaye

Reputation: 9756

A Stream map/reduce version of the loop for fun. Not sure if it is better to be honest. But at least we get rid of all the variables

  • map each item to a list of 3 booleans (one for each attribute x,y,z)
  • reduce the whole list into a list of 3 booleans (one for each attribute x,y,z) checking if any of the items has each value
  • check that all the elements of the the resulting list are true.
boolean allGood = items.stream()
    .map(i -> Arrays.asList(doesHaveX(i), doesHaveY(i), doesHaveZ(i)))
    .reduce(Arrays.asList(false, false, false),
            (acc, elem) -> Arrays.asList(acc.get(0) || elem.get(0), 
                                         acc.get(1) || elem.get(1), 
                                         acc.get(2) || elem.get(2)))
    .stream()
    .allMatch(Boolean::booleanValue);

Upvotes: 0

roookeee
roookeee

Reputation: 1818

Here is an extendable approach that uses an enum so you never have to touch the logic of hasOneOfAll again. You just have to extend the given enum.

import java.util.EnumMap;
import java.util.List;
import java.util.Map;
import java.util.function.Predicate;

class StackOverflowQuestion56902308Scratch {
    class ItemType {
        boolean x;
        boolean y;
        boolean z;
    }

    enum ItemTypeCheck implements Predicate<ItemType> {
        HASX() {
            @Override
            public boolean test(ItemType itemType) {
                //TODO: implement me
                return itemType.x;
            }
        },
        HASY() {
            @Override
            public boolean test(ItemType itemType) {
                //TODO: implement me
                return itemType.y;
            }
        },
        HASZ() {
            @Override
            public boolean test(ItemType itemType) {
                //TODO: implement me
                return itemType.z;
            }
        }
    }

    public static boolean hasOneOfAll(List<ItemType> itemTypes) {
        Map<ItemTypeCheck, Boolean> result = new EnumMap<>(ItemTypeCheck.class);
        for (ItemType itemType : itemTypes) {
            for (ItemTypeCheck check : ItemTypeCheck.values()) {
                result.merge(check, check.test(itemType), Boolean::logicalOr);
            }
        }

        return result.values().stream().allMatch(hadOne -> hadOne);
    }

}

Personally I am not sure if this is too overengineered but it alleviates the pain of manually adjusting the function if another check is added in the future.

Upvotes: 0

dbl
dbl

Reputation: 1109

If you are going to stick to a JVM below 1.8 then your code is just fine!

Maybe you could skip few operations like breaking the loop once you found a match for the three booleans, and checking only those which are not found any yet.

    for (ItemType item : items) {

        hasX = hasX || doesHaveX(item);
        hasY = hasY || doesHaveY(item);
        hasZ = hasZ || doesHaveZ(item);

        if (hasX && hasY && hasZ) {

            break;
        }
    }

If you are just fine to use streams maybe it's better to initialize each of the variables at it's creation like so:

    boolean hasX = items.stream().anyMatch(this::doesHaveX); // goes trough the elements until a match is found.
    boolean hasY = items.stream().anyMatch(this::doesHaveY); // goes trough the elements until a match is found.
    boolean hasZ = items.stream().anyMatch(this::doesHaveZ); // goes trough the elements until a match is found.

Upvotes: 1

Related Questions