#SharePoint Daily WTF: Complexity
So yesterday I came accross a particular piece of code that I couldn’t accept. Not only that, I can’t really see how da hell someone has the ‘awesome’ idea of putting it together.
What amazes me the most is that it was clearly done by someone who had no idea what he/she was doing.
some people say I criticize too much but for god sake, I do that to myself and it is to keep on improving.
anyway, Have a look at the code:
<code>
private SPListItem GetPageDataByBrand()
{
if (string.IsNullOrEmpty(Brand))
{
Brand = new PolicyDao().FindByPolicyNumber(PolicyNo).Quote.Brand.Name;
bool catchADE;
foreach (SPWebApplication wa in SPWebService.ContentService.WebApplications)
{
foreach (SPSite sc in wa.Sites)
{
catchADE = sc.CatchAccessDeniedException;
sc.CatchAccessDeniedException = false;
try
{
SPFeature sf = sc.Features[new Guid("32b12f24-a926-4b66-92d6-43eb3011febd")];
}
catch (IndexOutOfRangeException) { continue; }
//Ignore the web apps which we do not have access to.
catch (SqlException) { continue; }
try
{
foreach (SPWeb sw in sc.AllWebs)
{
using (sw)
{
PublishingWeb pWeb = PublishingWeb.GetPublishingWeb(sw);
foreach (PublishingPage pp in pWeb.GetPublishingPages())
{
if (pp.Layout.Name.Contains(“QuickQuote”))
{
if (pp.ListItem["Brand"] as String == Brand)
{
return pp.ListItem;
}
}
}
}
}
}
catch (Exception e)
{
//TODO SP Logging
}
finally
{
sc.CatchAccessDeniedException = catchADE;
sc.Dispose();
}
}
}
}
}
}
</code>
As you can easily see it:
1) nests 4 loops which by itself is a big no! In this scenario it means that this code can potentially go through every single sharepoint page to find a particular one. My questions
- isn’t search meant to do that?
- what are you trying to acchieve? sadly enough I can’t ask the person who did it because this company is long gone from this environment.
- SharePoint environments can get quite big, that means this code could potentially iterate a million times.
2) What if a particular site isn’t a publishing web, is it going to work?
- why do you need a publishing web if you are just returning a list item anyway? Ok, I understood he/she wanted to see if layout was quickquote. So what?
3) Thanks for saying there must be logging….
4) What are you doing with that SPSiteCollection.Features????
5) Thanks for wrapping the SPSiteCollection with a ‘using’ statement
Sorry but I just can’t accept devs who do this kind of thing. Please, I reckon you should give up and become something else.
Two possible ways to solve that:
1) Replace that by search. Include your ‘Brand’ and ‘ContentType’ field as managed properties and that will do the trick just fine.
2) create a reference links list. That could allow some extra flexibility as you can add some other parameters to it and use it as a resources list. Given the nature of this solution I decided to go for that.
