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

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.

  • \$\begingroup\$ What type notifications have? What type notification ID have? \$\endgroup\$ – Sergey Berezovskiy Feb 27 '14 at 20:44
  • \$\begingroup\$ They are a custom type. \$\endgroup\$ – Sweeden Feb 27 '14 at 20:45
  • \$\begingroup\$ Are you sure? Maybe notifications is a List<T>? Maybe ID is something like integer? \$\endgroup\$ – Sergey Berezovskiy Feb 27 '14 at 20:47
  • \$\begingroup\$ Sorry, I misread. Notifications is a IEnumerable<T>. \$\endgroup\$ – Sweeden Feb 27 '14 at 20:50
  • 3
    \$\begingroup\$ Since SendEmail is executed for a side-effect, it's inherently non functional and you should not use call it from LINQ. \$\endgroup\$ – CodesInChaos Feb 28 '14 at 13:17

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.

  • \$\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 Feb 27 '14 at 20:53
  • 4
    \$\begingroup\$ I sure hope so - it's a very misleading name to have! \$\endgroup\$ – Mathieu Guindon Feb 27 '14 at 20:54
  • \$\begingroup\$ @sweeden I'll update this answer with an example when I have a minute. \$\endgroup\$ – Mathieu Guindon Feb 27 '14 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 Feb 28 '14 at 14:33
  • 1
    \$\begingroup\$ Why do you mention Task? How will that help here? \$\endgroup\$ – svick Feb 28 '14 at 14:48

From Stackoverflow directly, this was posted as an answer.

var lookup =  notifications.ToLookup(notification =>

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

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

  • 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 Feb 27 '14 at 20:58
  • \$\begingroup\$ I will look into your answer. Thanks for the help :)! \$\endgroup\$ – Sweeden Feb 27 '14 at 23:11

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


var successfulIDs = notifications.Where(n => _emailService.SendEmail(n.Message.Subject, 
                                  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)

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

  • \$\begingroup\$ What about the errorCount++ line? \$\endgroup\$ – ANeves Feb 28 '14 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 Feb 28 '14 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 Feb 28 '14 at 13:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.