1
\$\begingroup\$

This function is used to modify values in list List<GoogleUsageMapping> and prepare them for entering into billing system.

I'm hoping this function can be more simplified and prettified, logic works. Function needs to keep track of maxValue and if the maxValue is of type LOGICAL_SIZE. Would love to eliminate one of the for-loop if possible for instance.

private void checkForOverage(List<GoogleUsageMapping> records) {
    BigDecimal highestQuantity = new BigDecimal(0);
    GoogleUsageMapping recordHighestQuantity = null;
    for (GoogleUsageMapping record : records) {
        if (record.getUsage().getQuantity().compareTo(highestQuantity) > 0) {
            highestQuantity = record.getUsage().getQuantity();
            recordHighestQuantity = record;
        }
    }

    if (recordHighestQuantity.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
        s_logger.info("Overage scenario accord LOGICAL_SIZE: " + recordHighestQuantity.getUsage().getQuantity() + " greater than ALLOCATED_SIZE");
        for (GoogleUsageMapping record : records) {
            if (record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
                continue;
            }
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}

modified version:

private void checkForOverage(List<GoogleUsageMapping> records) {
    GoogleUsageMapping recordUsageHighestQuantity = Collections.max(records, Comparator.comparing(c -> c.getUsage().getQuantity()));

    if (recordUsageHighestQuantity.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
        s_logger.info("Overage scenario accord LOGICAL_SIZE: " + recordUsageHighestQuantity.getUsage().getQuantity() + " greater than ALLOCATED_SIZE");
        for (GoogleUsageMapping record : records) {
            if (record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
                continue;
            }
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}
\$\endgroup\$

closed as off-topic by dfhwze, esote, t3chb0t, Toby Speight, pacmaninbw Aug 28 at 13:08

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." – dfhwze, esote, t3chb0t, Toby Speight, pacmaninbw
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ How would you use this code? What library you are using for the input GoogleUsageMapping? The code you presented in its current form is not meaningfully reviewable. We require sufficient context to make a proper review. See What topics can I ask about? for reference. \$\endgroup\$ – dfhwze Aug 26 at 16:57
  • \$\begingroup\$ @dfhwze added some more description as you pointed out. \$\endgroup\$ – JonB Aug 26 at 17:04
  • 1
    \$\begingroup\$ Possible NullPointerException if given an empty list or a list with no records with a usage quantity greater than zero. Perhaps that can never happen, but not possible to tell without greater context. \$\endgroup\$ – AJNeufeld Aug 27 at 3:09
1
\$\begingroup\$

If you want to get rid of a for-loop just for code clarity, you could use a Stream and the max() function. (Assuming you're using Java 8+). You could also use Streams for the second for-loop.

Note however, it's not logically possible to avoid iterating through the list twice. You have to check every element to get the largest value, and again to set values on every item in the list.

I don't see you using highestQuantity, it's not really necessary since you have a pointer to recordHighestQuantity already.

\$\endgroup\$
  • \$\begingroup\$ good pointers, will take a look at it, thank you \$\endgroup\$ – JonB Aug 26 at 16:59

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