Refactor foreach statement to LINQ

Question

I have been working on a project and I'm actually refactoring some code. I have encountered myself with lots of foreach and if statements, which could be easily replace with LINQ.

But I have this code snippet, that I wonder how I could make it more functional style.

foreach (var notification in notifications)
{
    if (_emailService.SendEmail(notification.Message.Subject, notification.Message.Body, notification.Message.MailTo))
    {
        successNotificationIDs.AddRange(notification.ID);
    }
    else
    {
        errorCount++;
    }
}

The SendEmail method of the EmailService returns a bool. If its execution has been successfully, it will add an IEnumerable of Int to a declared collection (successNotificationsIDs). If not, I will increase the errorCount variable.

What type notifications have? What type notification ID have? — Sergey Berezovskiy
– Sergey Berezovskiy, Commented Feb 27, 2014 at 20:44
Are you sure? Maybe notifications is a List<T>? Maybe ID is something like integer? — Sergey Berezovskiy
– Sergey Berezovskiy, Commented Feb 27, 2014 at 20:47
Since SendEmail is executed for a side-effect, it's inherently non functional and you should not use call it from LINQ. — CodesInChaos
– CodesInChaos, Commented Feb 28, 2014 at 13:17

Mathieu Guindon · Accepted Answer · 2014-02-27 20:52:07Z

12

LINQ isn't a silver bullet. It stands for Language-INtegrated-Query, which allows querying objects.

Querying objects isn't something that's supposed to have side-effects. However this is precisely what you loop's body is doing.

Therefore, refactoring it to use LINQ, if at all possible, would make it much less readable than the foreach loop you have here.

You could always create a Task object for each notification, and then run that.

Now the loop body you have, is using AddRange to add what appears to be a single value. Unless notification.ID is an IEnumerable<whatever>, you should be using the Add method to add a single value to your successNotificationIDs.

edited Feb 27, 2014 at 20:52

answered Feb 27, 2014 at 20:46

Mathieu Guindon

75.6k18 gold badges194 silver badges468 bronze badges

\$\begingroup\$ ID is an IEnumerable<int> actually. I don't have access to the class, but it has already been informed to change its name. \$\endgroup\$

Sweeden
– Sweeden

2014-02-27 20:53:44 +00:00
Commented Feb 27, 2014 at 20:53
4

\$\begingroup\$ I sure hope so - it's a very misleading name to have! \$\endgroup\$

Mathieu Guindon
– Mathieu Guindon

2014-02-27 20:54:16 +00:00
Commented Feb 27, 2014 at 20:54
\$\begingroup\$ @sweeden I'll update this answer with an example when I have a minute. \$\endgroup\$

Mathieu Guindon
– Mathieu Guindon

2014-02-27 23:12:49 +00:00
Commented Feb 27, 2014 at 23:12
\$\begingroup\$ @Mat'sMug: It's not really that wrong to use LINQ in this instance. There are lots of useful times where a 'query' doesn't directly get or set a property. Even in the setter of some properties, sometimes there are event bindings that could be considered side effects. I think it is a personal preference of coding style, which is beneficial when a query can be easily stacked with other LINQ queries to reduce directly writing multiple foreach loops. \$\endgroup\$

grovesNL
– grovesNL

2014-02-28 14:33:27 +00:00
Commented Feb 28, 2014 at 14:33
1

\$\begingroup\$ Why do you mention Task? How will that help here? \$\endgroup\$

svick
– svick

2014-02-28 14:48:02 +00:00
Commented Feb 28, 2014 at 14:48

| Show 1 more comment

Sweeden · Accepted Answer · 2014-02-27 20:55:47Z

4

From Stackoverflow directly, this was posted as an answer.

var lookup =  notifications.ToLookup(notification =>
    _emailService.SendEmail(notification.Message.Subject, 
        notification.Message.Body,
        notification.Message.MailTo));

var successfulIDs = lookup[true].SelectMany(notification => notification.ID);
var errorCount = lookup[false].Count();

Interesting as I didn't know the ToLookup method :).

answered Feb 27, 2014 at 20:55

Sweeden

1496 bronze badges

1

\$\begingroup\$ This is a rather interesting approach, however I would seriously consider hitting an SMTP server as an async operation, especially with this kind of construct. \$\endgroup\$

Mathieu Guindon
– Mathieu Guindon

2014-02-27 20:58:52 +00:00
Commented Feb 27, 2014 at 20:58
\$\begingroup\$ I will look into your answer. Thanks for the help :)! \$\endgroup\$

Sweeden
– Sweeden

2014-02-27 23:11:26 +00:00
Commented Feb 27, 2014 at 23:11

Add a comment |

grovesNL · Accepted Answer · 2014-02-28 14:23:50Z

2

Why not just use Where and Select if you're just trying to get a list of successful IDs?

ie.

var successfulIDs = notifications.Where(n => _emailService.SendEmail(n.Message.Subject, 
                                                                     n.Message.Body,
                                                                     n.Message.MailTo)).
                                  Select(n => n.ID);

If the type of notifications isn't right then you should be able to use Cast to do that.

To check how many failed, if you're able to use Count:

long errorCount = notifications.Count() - successfulIDs.Count();

I don't know what your types are, but you may be able to use Length instead of Count if not. Otherwise Cast and then Count.

Additionally, here's the equivalent foreach in case you decide to stay with that.

foreach(var notification in notifications)
    if(_emailService.SendEmail(notification.Message.Subject,
                               notification.Message.Body,
                               notification.Message.MailTo))
        successNotificationIDs.AddRange(notification.ID);

...for which you should still be able to use the errorCount described above. Note that you can combine these into a single line (potentially sacrificing readability in this case).

edited Feb 28, 2014 at 14:23

answered Feb 28, 2014 at 6:08

grovesNL

3282 silver badges12 bronze badges

\$\begingroup\$ What about the errorCount++ line? \$\endgroup\$

ANeves
– ANeves

2014-02-28 10:50:00 +00:00
Commented Feb 28, 2014 at 10:50
\$\begingroup\$ Yes. I have tried to go with that approach, but I need to inform the amount of notifications that failed to mailed. That's why I need the errorCount variable. \$\endgroup\$

Sweeden
– Sweeden

2014-02-28 12:54:51 +00:00
Commented Feb 28, 2014 at 12:54
\$\begingroup\$ @Sweeden: Why do you need a separate variable to increase that count? You can likely just compare the Count of both. I've edited my answer. \$\endgroup\$

grovesNL
– grovesNL

2014-02-28 13:11:35 +00:00
Commented Feb 28, 2014 at 13:11

Add a comment |

Stack Exchange Network

Refactor foreach statement to LINQ

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Refactor foreach statement to LINQ

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions