Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

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

3 Answers 3

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.

share|improve this answer
    
ID is an IEnumerable<int> actually. I don't have access to the class, but it has already been informed to change its name. –  Sweeden Feb 27 at 20:53
4  
I sure hope so - it's a very misleading name to have! –  Mat's Mug Feb 27 at 20:54
    
@sweeden I'll update this answer with an example when I have a minute. –  Mat's Mug Feb 27 at 23:12
    
@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. –  grovesNL Feb 28 at 14:33
1  
Why do you mention Task? How will that help here? –  svick Feb 28 at 14:48

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

share|improve this answer
1  
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. –  Mat's Mug Feb 27 at 20:58
    
I will look into your answer. Thanks for the help :)! –  Sweeden Feb 27 at 23:11

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

share|improve this answer
    
What about the errorCount++ line? –  ANeves Feb 28 at 10:50
    
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. –  Sweeden Feb 28 at 12:54
    
@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. –  grovesNL Feb 28 at 13:11

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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