0
\$\begingroup\$

Not please with this outcome, function works as is, but would love to simplify and prettify it. Use of one for-loop would be great and limit the use of the conditional operator.

private void checkForOverage(List<GoogleUsageMapping> records) {
    Long sumOfAllocatedSize = 0L;
    Long sumOfLogicalSize = 0L;
    for (GoogleUsageMapping record : records) {
        // initially all records are set to GoogleTrackingState.UNPROCESSED
        if (record.getUsage().getMeasuredType() == MeasuredType.ALLOCATED_SIZE) {
            sumOfAllocatedSize += record.getUsage().getQuantity().longValue();
        } else if (record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
            sumOfLogicalSize += record.getUsage().getQuantity().longValue();
        }
    }

    for (GoogleUsageMapping record : records) {
        if (sumOfAllocatedSize >= sumOfLogicalSize && record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        } else if (sumOfLogicalSize > sumOfAllocatedSize && record.getUsage().getMeasuredType() == MeasuredType.ALLOCATED_SIZE) {
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 2 → 1 \$\endgroup\$ Aug 28, 2019 at 18:49
  • 2
    \$\begingroup\$ You should really add more context so that we can give you proper advice. What class does this method appear in, and what does the code for the GoogleUsageMapping class look like? See How to Ask. (Our rules allow adding code for contextual information, but not revising the code that you have already posted.) \$\endgroup\$ Aug 28, 2019 at 21:03

2 Answers 2

2
\$\begingroup\$

If you have not done so. I recommend picking up the book Refactoring: Improving the Design of Existing Code by Martin Fowler. It is an excellent book on exactly this topic.

One thing you can start with is to split the method by extracting each operation into a separate method. So that instead of doing everything at once you end up with a method per operation.

Doing so will let you examine what your code is actually doing and helps you see the patterns. Something like this:

private void checkForOverage(List<GoogleUsageMapping> records) {
    long allocatedSize = sumAllocated(records);
    long logicalSize = sumLogical(records);
    checkAllocated(records,allocatedSize, logicalSize);
    checkLogical(records,allocatedSize, logicalSize);
}

private long sumAllocated(List<GoogleUsageMapping> records) {
    long sumOfAllocatedSize = 0L;
    for (GoogleUsageMapping record : records) {
        if (record.getUsage().getMeasuredType() == MeasuredType.ALLOCATED_SIZE) {
            sumOfAllocatedSize += record.getUsage().getQuantity().longValue();
        }
    }
    return sumOfAllocatedSize;
}

private long sumLogical(List<GoogleUsageMapping> records) {
    long sumOfLogicalSize = 0L;
    for (GoogleUsageMapping record : records) {
        if (record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
            sumOfLogicalSize += record.getUsage().getQuantity().longValue();
        }
    }
    return sumOfLogicalSize;
}

private void checkAllocated(List<GoogleUsageMapping> records, long allocatedSize, long logicalSize) {
    for (GoogleUsageMapping record : records) {
        if (allocatedSize >= logicalSize && record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}

private void checkLogical(List<GoogleUsageMapping> records, long allocatedSize, long logicalSize) {
    for (GoogleUsageMapping record : records) {
        if (logicalSize > allocatedSize && record.getUsage().getMeasuredType() == MeasuredType.ALLOCATED_SIZE) {
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}

As you can see both pairs of methods are nearly identical. All you have to do to unify them is to extract the difference as parameter.

private void checkForOverage(List<GoogleUsageMapping> records) {
    long allocatedSize = sumByType(records, MeasuredType.ALLOCATED_SIZE);
    long logicalSize = sumByType(records, MeasuredType.LOGICAL_SIZE);
    check(records, allocatedSize >= logicalSize, MeasuredType.ALLOCATED_SIZE);
    check(records, logicalSize > allocatedSize, MeasuredType.LOGICAL_SIZE);
}

private long sumByType(List<GoogleUsageMapping> records, MeasuredType type) {
    long sumOfAllocatedSize = 0L;
    for (GoogleUsageMapping record : records) {
        if (record.getUsage().getMeasuredType() == type) {
            sumOfAllocatedSize += record.getUsage().getQuantity().longValue();
        }
    }
    return sumOfAllocatedSize;
}

private void check(List<GoogleUsageMapping> records, boolean condition, MeasuredType type) {
    for (GoogleUsageMapping record : records) {
        if (condition && record.getUsage().getMeasuredType() == type) {
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ This is definitely less efficient. At the very least I would move the condition check in check outside of the loop, if the condition is false, nothing needs to be done at all. \$\endgroup\$
    – konijn
    Aug 28, 2019 at 13:33
  • 5
    \$\begingroup\$ @konijn Sure. Minor optimizations can be done all over the place, but that was not the point of this example. \$\endgroup\$
    – Januson
    Aug 28, 2019 at 13:53
  • 2
    \$\begingroup\$ The difference is substantial for large data volumes. \$\endgroup\$
    – konijn
    Aug 28, 2019 at 14:04
  • \$\begingroup\$ thank you both for you contribution, very helpful, please see my newest editing, where I'm at a solution that I really like. \$\endgroup\$
    – JonB
    Aug 28, 2019 at 16:24
1
\$\begingroup\$

After around of iterations I have come up with this solution, and I'm pretty please with it. I had introduced a static class as a some kind of data container for min, max and MeasuredType variables, but when completed with that iteration I saw clearly that code could be more simplified. So by abstracting code to more functions lead me to my final solution.

private void checkForOverage(List<GoogleUsageMapping> records) {
    MeasuredType measuredType = sumByMeasuredType(records);
    for (GoogleUsageMapping record : records) {
        if (record.getUsage().getMeasuredType() == measuredType) {
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}

private MeasuredType sumByMeasuredType(List<GoogleUsageMapping> records) {
    long allocatedSize = 0L;
    long logicalSize = 0L;
    for (GoogleUsageMapping record : records) {
        if (record.getUsage().getMeasuredType() == MeasuredType.ALLOCATED_SIZE) {
            allocatedSize += record.getUsage().getQuantity().longValue();
        } else if (record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
            logicalSize += record.getUsage().getQuantity().longValue();
        }
    }
    return (allocatedSize >= logicalSize) ? MeasuredType.LOGICAL_SIZE : MeasuredType.ALLOCATED_SIZE;
}
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.