Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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

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

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)));
Source Link
BCdotWEB
  • 11.4k
  • 2
  • 28
  • 45

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)));
lang-cs

AltStyle によって変換されたページ (->オリジナル) /