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\$

closed as off-topic by 200_success, Sᴀᴍ Onᴇᴌᴀ, Dannnno, dfhwze, Toby Speight Aug 29 at 7:23

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – 200_success, Sᴀᴍ Onᴇᴌᴀ, Dannnno, dfhwze
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 2 → 1 \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Aug 28 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\$ – 200_success Aug 28 at 21:03
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\$
  • 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 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 at 13:53
  • 2
    \$\begingroup\$ The difference is substantial for large data volumes. \$\endgroup\$ – konijn Aug 28 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 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.