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 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 theObjectToStringOrEmpty
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>
, useCount
instead ofCount()
. And use.Any()
instead of.Count > 0
. - Can
activeMembers
ever benull
? - I'm worried by
am.MembershipStatus == "Active" || am.MembershipStatus == "Pending")
: these look like magic strings to me, and thus they should beconst
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
Member
s, - 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)));
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 theObjectToStringOrEmpty
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>
, useCount
instead ofCount()
. And use.Any()
instead of.Count > 0
. - Can
activeMembers
ever benull
? - I'm worried by
am.MembershipStatus == "Active" || am.MembershipStatus == "Pending")
: these look like magic strings to me, and thus they should beconst
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
Member
s, - 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)));
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 theObjectToStringOrEmpty
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>
, useCount
instead ofCount()
. And use.Any()
instead of.Count > 0
. - Can
activeMembers
ever benull
? - I'm worried by
am.MembershipStatus == "Active" || am.MembershipStatus == "Pending")
: these look like magic strings to me, and thus they should beconst
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
Member
s, - 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)));
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 theObjectToStringOrEmpty
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>
, useCount
instead ofCount()
. And use.Any()
instead of.Count > 0
. - Can
activeMembers
ever benull
? - I'm worried by
am.MembershipStatus == "Active" || am.MembershipStatus == "Pending")
: these look like magic strings to me, and thus they should beconst
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
Member
s, - 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)));