Code Trivia: optimize the code for multiple nested loops

Question

I came across this code today and wondering what are some of the ways we can optimize it.

Obviously the model is hard to change as it is legacy, but interested in getting opinions.

Changed some names around and blurred out some core logic to protect.

private static Payment FindPayment(Order order, Customer customer, int paymentId)
    {
        Payment payment = Order.Payments.FindById(paymentId);
        if (payment != null)
        {
            if (payment.RefundPayment == null)
            {
                return payment;
            }

           if (String.Compare(payment.RefundPayment, "refund", true) != 0 )
            {
                return payment;
            }

        }

        Payment finalPayment = null;
        foreach (Payment testpayment in Order.payments)
        {
            if (testPayment.Customer.Name != customer.Name){continue;}

            if (testPayment.Cancelled) 
            {
                continue;
            }

            if (testPayment.RefundPayment != null) 
            {
                if (String.Compare(testPayment.RefundPayment, "refund", true) == 0 )
                {
                    continue;
                }
            }

            if (finalPayment == null)
            {
                finalPayment = testPayment;
            }
            else
            {
                if (testPayment.Value > finalPayment.Value)
                {
                    finalPayment = testPayment;
                }
            }
        }
       if (finalPayment == null) 
       {
           return payment;
       }

       return finalPayment;
    }

Making this a wiki so code enthusiasts can answer without worrying about points.

Answer 1

It's going to be highly data-dependent. You need to profile this with a "typical" data set, identify the bottlenecks, then consider appropriate optimisations based on your profile data.

Answer 2

 if (testPayment.Customer.Name != customer.Name){continue;}

That shouldn't be necessary for a start - surely all the payments against any given order relate to the same customer?

I don't like this function at all, if I'm passing a passing a payment_id, then I would only expect to get either that specific payment, or null... None of this searching around stuff...

Sounds to me like you need to think about redesigning a lot of code, and I think it goes well beyond this specific function...

Answer 3

If you can add members to the Payment class, add a didRefund boolean which is set to true whenever the RefundPayment string is set to "refund". This allows you to avoid the string compares.

Before the loop if you do initialize finalPayment like this:

finalPayment = new Payment;
finalPayment.Value = -1.0e12

then you can avoid the null test in the loop. (Assumes none of the customers are making negative billion dollar payments)

Answer 4

The first test (if (payment.RefundPayment == null)) is redundant. The second test using String.Compare works with null strings. You can use this "optimization" in the second place you use this comparison in the foreach loop.

Answer 5

You can move one of the conditions into a function of it's own since it's repeated twice (testing for it and its opposite). You can also shove the conditions together and wrap the whole loop in the first conditional. That way you only have one exit point for the function. If that doesn't seem manageable, you can wrap the foreach-loop into a function and just call it that way.

private static boolean IsRefundPayment(Payment payment) {
    return payment.RefundPayment != null && String.Compare(payment.RefundPayment, "refund", true) == 0;
}

private static Payment FindPayment(Order order, Customer customer, int paymentId) {
    Payment payment = Order.Payments.FindById(paymentId);
    if (payment == null || IsRefundPayment(payment)) {
        foreach (Payment testpayment in Order.payments) {
            if (testPayment.Customer.Name == customer.Name && !testPayment.Cancelled && !IsRefundPayment(payment)
                && (testPayment.Value > payment.Value)) {
                payment = testPayment;
            }
        }
    }
    return payment;
}