#What would be better when it comes to good practices and performance?

1 messages · Page 1 of 1 (latest)

eager hull
#

For small datasets, what would be better when it comes to good practices and performance?

Using streams with an AtomicBoolean (with the overhead it adds) like this:

    AtomicBoolean paymentFound = new AtomicBoolean(false);
    providerProduct.getProviderPayments().stream()
        .filter(payment -> payment.isCorporateCreditCard() && providerId.equals(payment.getPaymentCreditCard().getId()))
        .forEach(payment -> {
            if (!paymentFound.get()) {
                setPaymentValues(product, providerProduct, payment);
                paymentFound.set(true);
            }
            providerPayments.add(payment);
        });
}

Or avoid using the AtomicBoolean and have this instead:

private void calculateWhatever(Product product, ProviderProduct providerProduct) {
        boolean paymentFound = false;
        for (ProviderPayment payment : providerProduct.getProviderPayments()) {
            CreditCard creditCard = payment.getPaymentCreditCard();
            if (payment.isCorporateCreditCard() && providerId.equals(creditCard.getId())) {
                if (!paymentFound) {
                    setPaymentValues(product, providerProduct, payment);
                    paymentFound = true;
                }
                providerPayments.add(payment);
            }
        }
    }
cyan mistBOT
# eager hull For small datasets, what would be better when it comes to good practices and per...

Detected code, here are some useful tools:

Formatted code
private void calculateWhatever(Product product, ProviderProduct providerProduct) {
  AtomicBoolean paymentFound = new AtomicBoolean(false);
  providerProduct.getProviderPayments().stream().filter(payment -> payment.isCorporateCreditCard() && providerId.equals(payment.getPaymentCreditCard().getId())).forEach(payment -> {
    if (!paymentFound.get()) {
      setPaymentValues(product, providerProduct, payment);
      paymentFound.set(true);
    }
    providerPayments.add(payment);
  }
  );
}
#

<@&987246399047479336> please have a look, thanks.

civic oyster
#

This code it doing two things, adding everything that passes the filter to providerPayments and setting payment values (whatever that is) to the first one that passes the filter. If adding anything to providerPayments is dependent on setPaymentValues first of an arbitrary payment object then, to me, seems like poor design. code smell.

wanton raptor
#

This is nothing but personal opinion, so take it with a grain of salt, but Streams are best used without side effects, so I would opt to rather not call setPaymentValues from inside the stream.
What I would do is filter trough stream, collect it, check if I have found anything, call the setPaymentMethods on first and addAll on the wholr collection. Calling external methods with affects outside of the stream does not sit well with me.

eternal portal
#

To be honest, I like neither. Lambdas should be side effect free. And the naming of the atom variable is also a bit odd since you are not looking for payments, but corporate payments, so seemingly there could have been a private one.

mossy relic
#

there is almost certainly a better way to do the stream and with way fewer side effects.

civic oyster
#

There's more going on here that's smelly than the issue of whether to use a for loop or streams. There is obviously global or member variable state in the variable providerPayments along with the dubious setPaymentValues being required on the existence of an arbitrary payment. It frankly looks like ownership of state is adhoc. providerProduct owns payments yet providerPayments is owned elsewhere? I'm sure more visibility might pinpoint the real issue but from just this example, it looks like 'painted into a corner' kind of code.

mossy relic
#

yeah i also have no clue where providerPayments comes from

#

is that a field?

#

if so use this.

eternal portal
#

And the distinction between a product and providerproduct.