2
\$\begingroup\$

I have the following, quite ugly statement - is there a better way to write something of this nature?

if (!item["Created By"].ToString().Contains("System Account") &&
    !item.Url.Contains("_catalogs") &&
    !item.Url.Contains("Style Library") &&
    !item.Url.Contains("Cache Profiles") && 
    !item.ContentType.ToString().Contains("Theme Gallery") &&
    !item.ContentType.ToString().Contains("Converted Forms") &&
    !item.ContentType.ToString().Contains("Page Output Cache") &&
    !item.ContentType.ToString().Contains("Master Page"))
{
    query.RecordItems(item.ID.ToString(), item.ContentType.Name,
                        item.DisplayName, item.Name,
                        "", item.Url, item["Created By"].ToString(),
                        "",
                        Convert.ToDateTime(item["Modified"]),
                        item["Modified By"].ToString(),
                        Convert.ToDateTime(item["Created"]),
                        item["Created By"].ToString());
}

For a frame of reference, here's the proceeding code:

using (var web = activeSiteCollection.OpenWeb())
{
    foreach (SPList list in web.Lists)
    {
        query.RecordLists(list.ID.ToString(), list.Author.ToString(), list.Title,
                            list.DefaultViewUrl,
                            list.ParentWeb.Title, list.ParentWebUrl, list.ItemCount,
                            list.LastItemModifiedDate, list.LastItemDeletedDate);

        // check if files exist in the document library, if they don't break out to next level
        if (list.ItemCount <= 0) continue;

        for (int i = 0; i < list.Items.Count; i++)
        {
            try
            {
                SPListItem item = list.Items[i];

                if (!item["Created By"].ToString().Contains("System Account") &&
                    !item.Url.Contains("_catalogs") &&
                    !item.Url.Contains("Style Library") &&
                    !item.Url.Contains("Cache Profiles") && 
                    !item.ContentType.ToString().Contains("Theme Gallery") &&
                    !item.ContentType.ToString().Contains("Converted Forms") &&
                    !item.ContentType.ToString().Contains("Page Output Cache") &&
                    !item.ContentType.ToString().Contains("Master Page"))
                {
                    query.RecordItems(item.ID.ToString(), item.ContentType.Name,
                                        item.DisplayName, item.Name,
                                        "", item.Url, item["Created By"].ToString(),
                                        "",
                                        Convert.ToDateTime(item["Modified"]),
                                        item["Modified By"].ToString(),
                                        Convert.ToDateTime(item["Created"]),
                                        item["Created By"].ToString());
                }
            }
            catch (NullReferenceException)
            {
                SPListItem item = list.Items[i];

                Logger.Error(
                    "[{0}] Filed moving on file {1} as not all content was present",
                    item.Name);
            }

        }
\$\endgroup\$
5
\$\begingroup\$

As well as some standard indenting, a quick thought could be reversing the item being compared and using some temporary variables for readability.

EDIT:
Ok, so svick pointed out that the original code was fundamentally flawed. So how about creating an extension method on string to do the Contains for us. Something like:

// Method would do well with some unit tests ....
public static bool ContainsAny(this string source, string[] findStr)
{
    return findStr.Any(p => source.Contains(p));
}

Then used like

var contentTypes = new [] { "Theme Gallery", "Converted Forms", "Page Output Cache", "Master Page" };
var urls = new[] { "_catalogs", "Style Library", "Cache Profiles" };
var accounts = new [] { "System Account" };

var contentType = item.ContentType.ToString();
var createdBy = item["Created By"].ToString();

if (!createdBy.ContainsAny(accounts) &&
    !item.Url.ContainsAny(urls) &&
    !contentType.ContainsAny(contentTypes))
{
    query.RecordItems(
            item.ID.ToString(), 
            item.ContentType.Name,
            item.DisplayName, 
            item.Name,
            "", 
            item.Url, 
            createdBy,
            "",
            Convert.ToDateTime(item["Modified"]),
            item["Modified By"].ToString(),
            Convert.ToDateTime(item["Created"]),
            createdBy);
}

Also, do you need to catch the exception in this method? Could you not just handle that situation in an if statement as it looks like you are letting the exception handle the case where one of the values is null when a simple null check would suffice??

\$\endgroup\$
  • \$\begingroup\$ +1 I don't think the exception has been tested. The Logger.Error has a {0} and a {1} with only one argument. \$\endgroup\$ – dcaswell Oct 2 '13 at 3:52
  • \$\begingroup\$ Very nice solution. I was actually midway through changing the logging into events when I posted this, which is why there was an argument missing at the time. \$\endgroup\$ – Michael A Oct 2 '13 at 4:02
  • \$\begingroup\$ @ElvisLikeBear A small suggestion. If you let the question be unaccepted for a few days you are more likely to get other alternative or better suggestions, or even improvements on what I've done. Just a suggestion... \$\endgroup\$ – dreza Oct 2 '13 at 6:27
  • \$\begingroup\$ But this doesn't work the same as the original code. That tests for example for URLs that contain "_catalogs" as a substring, but you're checking only for URLs that are exactly "_catalogs". Also, saving ContentType to url is extremely confusing (and I think that because of that bad naming, you're comparing the wrong values). \$\endgroup\$ – svick Oct 2 '13 at 7:49
  • \$\begingroup\$ @svick yep, appears you might be onto something. I knew there was a reason I didn't want it accepted :) Back to the drawing board me thinks. \$\endgroup\$ – dreza Oct 2 '13 at 8:49

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.