CODEWITHSUNDEEP
c#linq
Overly Excessive
Overly Excessive

Reputation: 2125

Converting nested loop to LINQ statement

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();

Upvotes: 0

Views: 278

Answers (3)

p.s.w.g
p.s.w.g

Reputation: 149020

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

  1. 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).
  2. 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.

Upvotes: 2

zmbq
zmbq

Reputation: 39013

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.

Upvotes: 2

R&#233;da Mattar
R&#233;da Mattar

Reputation: 4381

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();

Upvotes: 2

Related Questions