How can I improve my method to get filtered data from user profile objects?

Question

I've hacked the following code together to get some details from my users' profile data. It works, but isn't pretty.

I have a UserPreferences object that is saved in the users' profiles. It has properties for whether SMS alert messages are desired and for which alert types. The SMS will be sent as an email. When an alert is created, I want to get a comma-delimited string of the addresses that should get notified for that alert type.

The user has a mobile number and a carrier. The carriers have an SMS email format like "{number}@ {domain} and I'm replacing those fields with the user's number and the carrier's domain.

As I said, this does work, but I don't care for it being two methods or how messy it seems. Is there a more compact way of writing this as one method?

public static string GetSMSAlertSubscribers(int alertTypeId) {
  // end result I want is like: "1234567890@vtext.com,0987654321@text.att.net"
  return String.Join(",", GetSMSAlertSubscribersEnumerable(alertTypeId));
}

public static IEnumerable<string> GetSMSAlertSubscribersEnumerable(int alertTypeId) {
  var prefs = Membership.GetAllUsers()
                        .Cast<MembershipUser>()
                        .Where(u => WebProfile.GetProfile(u.UserName).Preferences.SendAlertsToSMS.Equals(true)
                                 && WebProfile.GetProfile(u.UserName).Preferences.SMSAlertTypeIds.Contains(alertTypeId))
                        .Select(u => WebProfile.GetProfile(u.UserName).Preferences);

  var carriers = new AlertRepository().FindAllMobileCarriers().ToList();

  foreach (UserPreferences p in prefs) {
    yield return carriers.Where(c => c.Id.Equals(p.MobileCarrierId))
                         .Select(c => c.SMSEmailAddressFormat.Replace("{domain}", c.SMSEmailAddressDomain).Replace("{number}", p.MobileNumber))
                         .SingleOrDefault();
  }
}

I'd call it like:

var emailTo = GetSMSAlertSubscribers(3);

UPDATE: I've refactored my "prefs" query to reduce the repetitive use of GetProfile() as:

var prefs = Membership.GetAllUsers()
              .OfType<MembershipUser>()
              .Select(u => WebProfile.GetProfile(u.UserName).Preferences)
              .Where(p => p.SendAlertsToSMS && p.SMSAlertTypeIds.Contains(alertTypeId));

Then you can use just `c.id == p.MobileCarrierId`, and `SendAlertToSMS == true && ..` (or just `SendAlertToSMS && ..`. `Object.Equals(object)` isn't a good method to use — abatishchev, Apr 09 '11 at 08:41
@abatishchev: Thanks, I'll change it in my actual code. I probably used `Equals` only because looks cleaner and reads better. — JustinStolle, Apr 09 '11 at 08:44

abatishchev · Accepted Answer · 2011-04-09T09:41:37.333

2

var q = from x in
            (from p in prefs
            from c in carriers
            where p.MobileCarrierId == c.Id
            select new
            {
                c.SMSEmailAddressFormat,
                c.SMSEmailAddressDomain,
                p.MobileNumber
            }).Distinct()
        select x.SMSEmailAddressFormat
            .Replace("{domain}", x.SMSEmailAddressDomain)
            .Replace("{number}", x.MobileNumber);

return String.Join(", ", q);

var q = from p in prefs
        from c in carriers
        where p.MobileCarrierId == c.Id
        select c.SMSEmailAddressFormat
           .Replace("{domain}", c.SMSEmailAddressDomain)
           .Replace("{number}", p.MobileNumber);

return String.Join(", ", q.Distinct());

edited Apr 09 '11 at 09:41

answered Apr 09 '11 at 08:40

abatishchev

98,240
88
296
433

Ah, nice -- no more loop! This approach seems more flexible or easier to follow if I later wanted to format the addresses like "First Last" . Thank you, sir. – JustinStolle Apr 09 '11 at 09:12
I was thinking whether I really needed `Distinct()` in there, but I realized that two users might--for some odd reason, because nothing prevents it--have entered the same mobile number, and that would help ensure the message is sent only once per number. – JustinStolle Apr 09 '11 at 09:25
@Justin: You can distinct both preferences and result. I updated my answer. And the second one seems to me to be more reliable and readable. – abatishchev Apr 09 '11 at 09:40

score 1 · Answer 2 · answered Apr 07 '11 at 22:31

1

Mh, how about this:

var prefs = Membership
                .GetAllUsers()
                .OfType<MembershipUser>()
                .Select(u => WebProfile.GetProfile(u.UserName).Preferences)
                .Where(p => SendAlertToSMS.Equals(true) && SMSAlertTypeIds.Contains(alertTypeId));

var carriers = new AlertRepository().FindAllMobileCarriers().ToList();

foreach (UserPreferences p in prefs) 
{
    var carrier = carriers.FindOrDefault(c => c.Id.Equals(p.MobileCarrierId));
    yield return PopulateAddress(c, p);
}

And do the replace stuff in PopulateAddress. Looks less messy to me now. Cramming everything into one Linq query is not neccessarily the best thing to do.

answered Apr 07 '11 at 22:31

ChrisWue

18,612
4
58
83

Maybe that's as good as it gets. Do you think this code could benefit from using `SelectMany()` anywhere? – JustinStolle Apr 07 '11 at 23:57
Depends in you definition of "benefit". The code this way is quite clear (imho): First get all the user preferences which want to be alerted by SMS for a specific alarm and then you build the actual addresses from the templates. Now you might be able to pull it all of in one Linq chain with `SelectMany` but I doubt if someone comes back to it 6 months time he will understand immediately what it's actually doing. – ChrisWue Apr 08 '11 at 01:52
How funny, I didn't realize you had switched the order of `Select` and `Where` until after I did it on my own. Belated props. – JustinStolle Apr 09 '11 at 10:13

How can I improve my method to get filtered data from user profile objects?

2 Answers2