12
\$\begingroup\$

I am getting two different lists of members, then if certain conditions meet, I add more members to the list before converting into an array.

Is there any way this could be improve? I guess we can use Linq and cast but I am not advanced in either of the skills I mentioned.

    List<Member> Members = new List<Member>();
    foreach (SPListItem mItem in GetList(Url).Items)
    {
        Member m = new Member();
        m.ID = mItem.ID;
        m.Name = mItem.Title;
        m.Company = Utilities.ObjectToStringOrEmpty(mItem[companyCol]);
        m.eMail = Utilities.ObjectToStringOrEmpty(mItem[emailCol]);
        m.Comment = Utilities.ObjectToStringOrEmpty(mItem[commentCol]);
        m.Membership = Utilities.ObjectToStringOrEmpty(mItem[msCol]);
        Members.Add(m);
    }

    if (DateTime.Now < row.EndDate)
    {
        var cd = new MemberManager().GetMoreMembers(Url + "/");
        var activeMembers = cd.Where(am => am.MembershipStatus == "Active" || am.MembershipStatus == "Pending").ToList();
        if (activeMembers != null || activeMembers.Count() > 0)
        {
            foreach (var am in activeMembers)
            {
                if (!Members.Any(a => a.eMail.ToLowerInvariant() == am.Email.ToLowerInvariant()))
                {
                    Member m = new Member();
                    m.Name = am.FirstName + " " + am.LastName;
                    m.eMail = am.Email;
                    m.IsVip = true;
                    Members.Add(m);
                }
            }
        }
    }

    md.Members = Members.ToArray();
\$\endgroup\$
  • \$\begingroup\$ Is this a method? If so, please include the method's signature (the parameters and all) \$\endgroup\$ – IEatBagels Oct 14 '15 at 13:51
  • \$\begingroup\$ @TopinFrassi it's a large method of a live web services and I am not allowed to touch anything else to be honest. \$\endgroup\$ – Mathematics Oct 14 '15 at 13:52
  • 1
    \$\begingroup\$ I think the question as-is is fine. I just thought it'd be easier to review with the full method but that's fine \$\endgroup\$ – IEatBagels Oct 14 '15 at 13:57
  • 2
    \$\begingroup\$ I'd also mention that if your method is any longer than that, it's very likely doing too many things. One shouldn't be afraid to refactor poorly written code. \$\endgroup\$ – Mathieu Guindon Oct 14 '15 at 17:42
  • \$\begingroup\$ @Mat'sMug you are right, services were designed very poorly and because they are already rolled out it's difficult for me to refactor them, but yes it definitely should be broken down into several methods, totally agree. \$\endgroup\$ – Mathematics Oct 15 '15 at 6:01
11
\$\begingroup\$

You can shorten the following snippet by using LINQ and a projection:

List<Member> Members = new List<Member>();
foreach (SPListItem mItem in GetList(Url).Items)
{
    Member m = new Member();
    m.ID = mItem.ID;
    m.Name = mItem.Title;
    m.Company = Utilities.ObjectToStringOrEmpty(mItem[companyCol]);
    m.eMail = Utilities.ObjectToStringOrEmpty(mItem[emailCol]);
    m.Comment = Utilities.ObjectToStringOrEmpty(mItem[commentCol]);
    m.Membership = Utilities.ObjectToStringOrEmpty(mItem[msCol]);
    Members.Add(m);
}

becomes

List<Member> Members = GetList(Url).Items.Select(item => new Member { 
                                                        ID = item.ID, 
                                                        Name = item.Title}).ToList();  
// Do this for every field you're interested in

var cd = new MemberManager().GetMoreMembers(Url + "/");

If a method doesn't require instance-level information, you should make it static. It will save you another object allocation and it just makes more sense to write MemberManager.GetMoreMembers() then.


am.MembershipStatus == "Active" || am.MembershipStatus == "Pending"

This would make more sense as an enum rather than a string. There's only a limited amount of values that status can be.


if (activeMembers != null || activeMembers.Count() > 0)

Boolean logic! You mean to use

if (activeMembers != null && activeMembers.Count() > 0)

a.eMail.ToLowerInvariant() == am.Email.ToLowerInvariant()

This will create 2 new string objects every iteration. Instead use string.Equals(a.eMail, am.Email, StringComparison.InvariantCultureIgnoreCase);

Notice also the eMail and Email discrepancy.


You can also shorten the above block by using

var newMembers = activeMembers.Where(activeMember => 
                    !Members.Any(member => string.Equals(activeMember.eMail, 
                                                          member.Email, 
                                                          StringComparison.InvariantCultureIgnoreCase))
                            .Select(newMember => new Member { 
                                                    Name = newMember.FirstName + " " + newMember.LastName,
                                                    eMail = newMember.Email,
                                                    IsVip = true
                                                });

Members = Members.Concat(newMembers);

The above is written without any IDE but I think you can figure out the solution to any syntax errors in it.

\$\endgroup\$
  • \$\begingroup\$ Your answer is complete but now requirements are bit changed, I need to update member's VIP property to true, if "!Members.any(..." false, i may ask another question later on \$\endgroup\$ – Mathematics Oct 15 '15 at 8:00
10
\$\begingroup\$

Some quick remarks:

  • Bad naming: what is cd, am, m, mItem,...?
  • Members should be camelCase.
  • Do not do this a.eMail.ToLowerInvariant() == am.Email.ToLowerInvariant(), instead use String.Equals.
  • Don't name your class Utilities, that's just asking for it to become an endless mess of various unrelated methods. I don't even see the point of the ObjectToStringOrEmpty method, since (string)mItem[companyCol] does the same thing and doesn't require me to go look in another class.
  • If you're working with a List<T>, use Count instead of Count(). And use .Any() instead of .Count > 0.
  • Can activeMembers ever be null?
  • I'm worried by am.MembershipStatus == "Active" || am.MembershipStatus == "Pending"): these look like magic strings to me, and thus they should be const in a static class. Moreover, they look like they should be enum values.

You say "it's a large method of a live web services" and it shows. In these 30-odd lines it:

  • gets data from a SharePoint server,
  • converts these entries to Members,
  • adds them to List<Member> Members,
  • then retrieves more data from another source (MemberManager)
  • and adds those to Members when appropriate.

These things should really be split up into smaller methods and possibly even be moved to separate classes.

If you move the Member creation inside foreach (SPListItem mItem in GetList(Url).Items) to a separate method e.g. ConvertToMember, you could replace the foreach with a LINQ chain:

var members = GetList(Url).Items.Select(ConvertToMember).ToList();

Same for the activeMembers logic, which could probably be reduced to something like this:

Members.AddRange(
    new MemberManager()
    .GetMoreMembers(Url + "/")
    .Where(am => (am.MembershipStatus == "Active" || am.MembershipStatus == "Pending")
        && CanBeAdded(am.Email)));
\$\endgroup\$
  • \$\begingroup\$ Thanks for useful comments, But "(string)mItem[companyCol] does the same thing" it doesn't, (string)object returns null not empty \$\endgroup\$ – Mathematics Oct 15 '15 at 6:36
  • \$\begingroup\$ @PleaseTeach True, but I'd argue that returning string.Empty would be a bad thing, and that the code that can't deal with a null string should be revised. \$\endgroup\$ – BCdotWEB Oct 15 '15 at 7:50
  • \$\begingroup\$ I agree, working on ex developers code, will see if I could improve it \$\endgroup\$ – Mathematics Oct 15 '15 at 7:54
  • \$\begingroup\$ why .Any() is better then Count() \$\endgroup\$ – Mathematics Oct 15 '15 at 8:48
  • \$\begingroup\$ @PleaseTeach stackoverflow.com/a/305156/648075 \$\endgroup\$ – BCdotWEB Oct 15 '15 at 8:58
5
\$\begingroup\$

I suggest just a tiny little change as there's already been said a lot.

For the exact same purpose as your utility method:

Utilities.ObjectToStringOrEmpty(mItem[companyCol])

I very often use an extension like this one:

static class ObjectExtensions
{
    public static string ToStringOrEmpty(this object value)
    {
        return value == null ? string.Empty : value.ToString();
    }
}

this will make you code look simpler:

m.Company = mItem[companyCol].ToStringOrEmpty();
\$\endgroup\$
-5
\$\begingroup\$

The following use IEnumerable Linq Expressions. Linq is about expression building. Most implementations you'll see use anonymous delegates defined locally; however, some expressions do not need to be defined within a function scope.

The PendingMember to Member Select Expression/Clause below, is one such expression (Note that PendingMember is the Mock Type I'm defining for the am type in your code snippet). It does not require/use an instance outside of it's anonymous function scope.

Mock Models

    public class SPListItem
    {
        public SPListItem()
        {
            Items = new System.Dynamic.ExpandoObject();
            Items.companyCol = "Anonymous Inc.";
            Items.emailCol = "noreply@anon.net";
            Items.commentCol = String.Empty;
            Items.msCol = "Anonymous";
        }

        public int ID { get; set; }
        public string Title { get; set; }
        public string this[string index]
        {
            get { return (string)(Items as IDictionary<string, object>)[index]; }
            set { (Items as IDictionary<string,object>)[index] = value; }
        }


        private dynamic Items { get; set; }

    }

    public class Member
    {
        public int ID { get; set; }
        public string Name { get; set; }
        public string Company { get; set; }
        public string eMail { get; set; }
        public string Comment { get; set; }
        public string Membership { get; set; }
        public bool IsVip { get; set; }

    }

    public class PendingMember
    {
        public int ID { get; set; }
        public string Email { get; set; }
        public string FirstName { get; set; }
        public string LastName { get; set; }
        public string MembershipStatus { get; set; }
    }

    public static dynamic GetList(string Url)
    {
        dynamic result = new System.Dynamic.ExpandoObject();

        var list = new List<SPListItem>(new SPListItem[]
        {
            new SPListItem() {
                 ID = 0, Title = "Title 0"
            },
            new SPListItem() {
                 ID = 1, Title = "Title 1"
            },
            new SPListItem() {
                 ID = 2, Title = "Title 2"
            },
            new SPListItem() {
                 ID = 3, Title = "Title 3"
            },
        });

        list.First()["emailCol"] = "jane.doe@doe.org";

        result.Items = list;
        return result;
    }

    public class MemberManager
    {
        public IEnumerable<PendingMember> GetMoreMembers(string url)
        {
            return new PendingMember[]
            {
                new PendingMember()
                {
                    FirstName = "John",
                    LastName = "Doe",
                    ID = 10,
                    Email = "john.doe@doe.org",
                    MembershipStatus = "pending"
                },
                new PendingMember()
                {
                    FirstName = "Jane",
                    LastName = "Doe",
                    ID = 1,
                    Email = "Jane.doe@doe.org",
                    MembershipStatus = "active"
                },
                new PendingMember()
                {
                    FirstName = "Jason",
                    LastName = "Bourne",
                    ID = 2,
                    Email = "jason.bourne@usa.gov",
                    MembershipStatus = "pending"
                }
            }.AsEnumerable();
        }
    }

    public static class Utilities
    {
        public static string ObjectToStringOrEmpty(string value)
        {
            return (String.IsNullOrEmpty(value)) ? "" : value;
        }
    }

Program Entry Point

    readonly static Func<PendingMember, Member> SelectPendingMemberToMemberClause = (active) => new Member()
    {
        Name = active.FirstName + " " + active.LastName,
        eMail = active.Email,
        IsVip = true
    };


    static void Main(string[] args)
    {
        string Url = "";
        string companyCol = "companyCol",
            emailCol = "emailCol",
            commentCol = "commentCol",
            msCol = "msCol";

        // this Function Delegate uses 'companyCol', 'emailCol', 'commentCol', 'msCol'.. so we're setting it locally.
        Func<SPListItem, Member> SelectSPListItemToMemberClause = mItem => new Member()
        {
            ID = mItem.ID,
            Name = mItem.Title,
            Company = Utilities.ObjectToStringOrEmpty(mItem[companyCol]),
            eMail = Utilities.ObjectToStringOrEmpty(mItem[emailCol]),
            Comment = Utilities.ObjectToStringOrEmpty(mItem[commentCol]),
            Membership = Utilities.ObjectToStringOrEmpty(mItem[msCol])
        };

        // the `<T> as IEnumerable<SPListItem> `cast is here only because I'm using dynamic object for the mocking.. 
        List<Member> members = (GetList(Url).Items as IEnumerable<SPListItem>).Select(SelectSPListItemToMemberClause).ToList();



        // if (DateTime.Now < row.EndDate) {

        // This Function Delegate uses 'members', so we're setting it locally...
        Func<PendingMember, bool> WhereActiveOrPendingStatusExcludingMembersClause = (active) => (active.MembershipStatus.ToLower() == "active" || active.MembershipStatus.ToLower() == "pending") && !(members.Any((member) => member.eMail.ToLowerInvariant() == active.Email.ToLowerInvariant()));


        IEnumerable<PendingMember> cd = new MemberManager().GetMoreMembers(Url + "/");


        members.AddRange(cd.Where(WhereActiveOrPendingStatusExcludingMembersClause)
            .Select(SelectPendingMemberToMemberClause).ToArray());


        //}

        // members.ToArray();

        // The next part prints to console, and is outside of the scope of this answer...

        members.ForEach(member =>
        {
            string formatString = @"Name:= {0}, ID:= {1}, Email:= {2}, Company:= {3}, Membership:= {4}, IsVIP:= {5}, Comment:= ""{6}""";
            Console.WriteLine(String.Format(formatString, member.Name, member.ID, member.eMail, member.Company, member.Membership, member.IsVip, member.Comment));
        });

        Console.WriteLine(value: "Press [Enter] to Proceed");
        ConsoleKey key;
        do
        {
            key = Console.ReadKey().Key;
        } while (key != ConsoleKey.Enter);
    }

OUTPUT

Name:= Title 0, ID:= 0, Email:= jane.doe@doe.org, Company:= Anonymous Inc., Membership:= Anonymous, IsVIP:= False, Comment:= ""
Name:= Title 1, ID:= 1, Email:= noreply@anon.net, Company:= Anonymous Inc., Membership:= Anonymous, IsVIP:= False, Comment:= ""
Name:= Title 2, ID:= 2, Email:= noreply@anon.net, Company:= Anonymous Inc., Membership:= Anonymous, IsVIP:= False, Comment:= ""
Name:= Title 3, ID:= 3, Email:= noreply@anon.net, Company:= Anonymous Inc., Membership:= Anonymous, IsVIP:= False, Comment:= ""
Name:= John Doe, ID:= 0, Email:= john.doe@doe.org, Company:= , Membership:= , IsVIP:= True, Comment:= ""
Name:= Jason Bourne, ID:= 0, Email:= jason.bourne@usa.gov, Company:= , Membership:= , IsVIP:= True, Comment:= ""
Press [Enter] to Proceed
\$\endgroup\$
  • 1
    \$\begingroup\$ What.. happened here? Why did the codebase multiply tenfold? What necessitated the introduction of dynamic (which is one of the most expensive operations you can do)? \$\endgroup\$ – Jeroen Vannevel Oct 14 '15 at 16:10
  • \$\begingroup\$ @JeroenVannevel, the answer to both questions should be obvious.. the code base includes mock types so it can be run, and dyanmic is a used to mock the SPListItem method which is apparently (and likely) used to retrieve info from a web service (which is a runtime consideration that uses IO and deserialization).. \$\endgroup\$ – Brett Caswell Oct 14 '15 at 16:41
  • 1
    \$\begingroup\$ It's not obvious. It reads like a code dump and I can't imagine this being very useful to OP. \$\endgroup\$ – RubberDuck Oct 14 '15 at 23:26

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.