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.

Are you looking for performance optimizations or readability optimizations? — Austin Salonen
– Austin Salonen, Commented Mar 26, 2010 at 16:52
LinQ might not give you an improvement in terms of performance, but it would almost certainly improve on readability. Shame you can't use it. — Martin Milan
– Martin Milan, Commented Mar 26, 2010 at 16:54
How many types of Payments do you have? Is it just Refund and "Normal"? — Austin Salonen
– Austin Salonen, Commented Mar 26, 2010 at 16:58
What version of VS are you using? Even if you are using .Net 2.0 from VS2008, you can easily fake LINQ. — Matthew Whited
– Matthew Whited, Commented Mar 26, 2010 at 19:14

Martin Milan · Accepted Answer · 2010-03-26 16:59:03Z

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...

answered Mar 26, 2010 at 16:59

community wiki

Martin Milan

Sign up to request clarification or add additional context in comments.

3 Comments

Austin Salonen Over a year ago

Depends on the nature of the app. I've seen business apps where an order can have multiple paying customers with varying percentages each one owes. Though comparing by name could cause issues if name isn't a PK.

Martin Milan Over a year ago

+1 for that, but given the code we have already seen I think it's unlikely. You're right to point it out though...

Srikar Doddi Over a year ago

I am just getting around the app..there is lot that needs to be done..I think I will get several opportunities to post trivia.

Paul R · Accepted Answer · 2010-03-26 16:47:06Z

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.

answered Mar 26, 2010 at 16:47

community wiki

Paul R

4 Comments

Paul R Over a year ago

I only see one loop (foreach) - perhaps you mean something other than nesting ?

Martin Milan Over a year ago

There's a lot of nested conditions in there, which could be removed by taking advantage of the fact that C#'s && and || operators are actually shortcut logical operators, similar to AndAlso and OrElse in VB. It wouldn't improve performance, but would make the code easier to follow. The importance of code readability is often underestimated...

Paul R Over a year ago

@Martin: sure - I was just questioning the OP's reference to "multiple nested loops", since there is only a single loop. I still think he should profile this first and then decide what to optimise, if anything.

Martin Milan Over a year ago

Agreed - though he should really look at a re-write... The idea of having the refund string for example - this would be a lot quicker with an enumeration...

AShelly · Accepted Answer · 2010-03-26 17:06:54Z

1

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)

answered Mar 26, 2010 at 17:06

community wiki

AShelly

Comments

Erich Mirabal · Accepted Answer · 2010-03-26 17:15:01Z

1

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.

answered Mar 26, 2010 at 17:15

community wiki

Erich Mirabal

Comments

2 revs user9903 · Accepted Answer · 2010-03-26 18:58:58Z

1

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;
}

edited Mar 26, 2010 at 18:58

community wiki

2 revs
user9903

1 Comment

Einar Over a year ago

Omit finalPayment, just write directly to payment. Then you'll lose a declaration, as well as the final if.

Collectives™ on Stack Overflow

Code Trivia: optimize the code for multiple nested loops

5 Answers 5

3 Comments

4 Comments

Comments

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

3 Comments

4 Comments

Comments

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related