Converting nested loop to LINQ statement

Question

I'm trying to convert this for loop :

for (int i = 100; i < 1000; i++)
{
    for (int j = 100; j < 1000; j++)
    {
        if (IsPalindrome(i * j))
        {
            palindromes.Add(i * j);
        }
    }
}

// For some reason the list is not sorted correctly, but when sorted it works.
palindromes.Sort();
Console.WriteLine(palindromes.Last());

Into a single LINQ statement, I'm messing up with the multiplications though, this is what I have so far, unfortunately it doesn't seem to increment correctly resulting in the wrong collection of numbers.

var palis = Enumerable.Range(100, 999)
                      .Select(n => n * Enumerable.Range(100, 999)
                                                 .Aggregate((ctr, num) => ctr++))
                      .Where(n => IsPalindrome(n)).Max();

Community · Accepted Answer · 2017-05-23 11:49:25Z

2

Réda Mattar's answer is close, but there are a few problems with it:

The second parameter to Enumerable.Range is the number of integers to return. This should be 900 (because your loop goes from 100 to 999).
It would be much more efficient to start from the high range and simply return the first palindrome found for each value of j rather than going through every possible combination of integers.

I'd suggest a slightly different form:

var maxPalindrome = 
    (from i in Enumerable.Range(1, 900)
     select (from j in Enumerable.Range(1, 900)
             let p = (1000 - i) * (1000 - j)
             where IsPalindrome(p)
             select p).FirstOrDefault()).Max();
Console.WriteLine(maxPalindrome);

But using Linq usually has overhead in terms of efficiency. This method could be made even more efficient by taking the advice mentioned above and simply rewriting your for-loop like this:

int maxPalindrome = 0;
for(int i = 999; i >= 0; i--) 
{
    for(int j = 999; j >= 0; j--) 
    {
        var p = i * j;
        if (p <= maxPalindrome) 
        {
            break;
        }
        if (IsPalindrome(p)) 
        {
            maxPalindrome = p;
            break;
        }
    }
}

Console.WriteLine(maxPalindrome);

A quick benchmark give the following results (over 10 trials):

Original code: 04.14s
Réda Mattar's method: 05.59s
My Linq method: 02.95s
My for-loop method: 0.04s

As you can see, the for loop gives the best performance. However, efficiency shouldn't be your only concern. In general you should choose the solution which you find easiest to read and maintain.

edited May 23, 2017 at 11:49

CommunityBot

11 silver badge

answered Apr 26, 2014 at 20:28

p.s.w.g

150k31 gold badges307 silver badges339 bronze badges

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

1 Comment

Overly Excessive Over a year ago

Like you said, in this case I'm not aiming for efficiency but rather readability and "elegance", where I find that LINQ expressions are usually more elegant than the traditional way.

Réda Mattar · Accepted Answer · 2014-04-26 20:14:32Z

2

Have you tried :

var palindromeMax = (from i in Enumerable.Range(100, 999)
                     from j in Enumerable.Range(100, 999)
                     where IsPalindrome(i * j)
                     select i * j).Max();

answered Apr 26, 2014 at 20:14

Réda Mattar

4,3911 gold badge20 silver badges19 bronze badges

Comments

zmbq · Accepted Answer · 2014-04-26 20:31:55Z

2

Don't do it. The resulting LINQ expression would be harder to understand than the code you started with. Take the other answers for instance - it's hard to get this LINQ expression right, and it's hard to understand exactly what it does.

Keep your own code - it's fine.

answered Apr 26, 2014 at 20:31

zmbq

39.1k15 gold badges109 silver badges188 bronze badges

Collectives™ on Stack Overflow

Converting nested loop to LINQ statement

3 Answers 3

1 Comment

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related