Get a value from a list in Sharepoint 2010

Question

I am using SharePoint 2010 and get values from a list using the code below.

It might be called like this: getValueFromList("addresses", "secondName", "city", "manhattan");

It will return the "secondName" of the first entry that has "city = manhattan". Is there a way to make it more efficient?

private string getValueFromList(string theList, string returnColumnName, string matchColumnName, string valueToMatch)
{
 try
 {
 SPSite siteh2 = SPContext.Current.Site;
 using (SPWeb oWeb = siteh2.OpenWeb())
 {
 //SPWeb oWeb = SPControl.GetContextSite(Context).OpenWeb();
 SPList oList = oWeb.Lists[theList];
 SPQuery oQuery = new SPQuery();
 oQuery.Query = "<Where><Eq><FieldRef Name='" + matchColumnName + "' />" +
 "<Value Type='Text'>" + valueToMatch + "</Value></Eq></Where>";
 //oQuery.Query = "<Query><Where><Eq><FieldRef Name='Username' /><Value Type='Text'>whelanp</Value></Eq></Where></Query>";
 SPListItemCollection results = oList.GetItems(oQuery);
 if (results.Count == 0)
 return "";
 foreach (SPListItem oListItem in results)
 {
 return cleanValue(oListItem[returnColumnName].ToString());
 }
 return "";
 }
 }
 catch (Exception ex)
 {
 return "";
 }
}

• BCdotWEB: FYI if you haven't seen it: sharepoint.stackexchange.com
• Abbas: As this is a review of code, wheter it is for SP or not, I think this is a valid question.

Answer 1

Naming conventions:

As per the Microsoft guidelines, use PascalCase for method names. Read more here: Capitalization Conventions. Your method names will become GetValueFromList and CleanValue.

Object Initialization and Format String:

Use the String.Format method to build your string, don't use concatenation. The building of your CAML query will be:

var spQuery = new SPQuery
{
 Query = String.Format("<Where><Eq><FieldRef Name='{0}' /><Value Type='Text'>{1}</Value></Eq></Where>", matchColumnName , valueToMatch)
};

More reading on Object and Collection Initializers.

Get SharePoint list:

Don't blindly use web.Lists["listName"], it depends on the number of lists you have in your environment what call you should use.

Using web.Lists["listName"], SharePoint will look for all lists in the current web-object and fetch all metadata from them. Then it compares the name you provided and return the correct list.

And SPWeb.GetList("url/to/list") will make a call to the DB for each list for which info is needed.

If you have a lot of lists (thousands), use the second method, otherwise the first will normally score better. Also, this depends on the performance of your hardware. But in general, I tend to avoid the indexer (using the second method or even web.TryGetList("listTitle")).

The foreach loop:

You have this piece of code:

foreach (SPListItem oListItem in results)
{
 return cleanValue(oListItem[returnColumnName].ToString());
}

Why have a loop if you will return on the first item? If you only want to return the first entry, use the SPQuery.RowLimit property and set it to 1:

var spQuery = new SPQuery
{
 Query = String.Format("<Where><Eq><FieldRef Name='{0}' /><Value Type='Text'>{1}</Value></Eq></Where>", matchColumnName , valueToMatch),
 RowLimit = 1
};

Following code:

SPListItemCollection results = oList.GetItems(oQuery);
if (results.Count == 0)
 return "";
foreach (SPListItem oListItem in results)
{
 return cleanValue(oListItem[returnColumnName].ToString());
}
return "";

will now look like this:

var results = list.GetItems(spQuery);
if (results.Count == 0)
 return "";
var item = results[0];
return CleanValue(item[returnColumnName].ToString());

Or in one return statement:

return results.Count > 0 ? CleanValue(item[returnColumnName].ToString()) : String.Empty;

Nitpicking:

• Variable names:
  • theList should be replaced by listName or listTitle.
  • Avoid adding the o before the names, use spList and spQuery instead.

Answer 2

Method names in .net should be PascalCase, so that would be GetValueFromList (and CleanValue), consistent with the rest of the framework and with what .net devs expect to see; Hungarian-style prefixes like the o of oWeb and oList are annoying too - nobody cares that an object is an Object. Name things after what they're used for, not for their type.

It's not clear what theList is supposed to be. From the parameter name I'd deem it a List<string> perhaps, and then source would have been a better name - but then this becomes surprising:

SPList oList = oWeb.Lists[theList];

Assuming the SPListCollection indexer uses a list's Title property, it seems title would be a more accurate parameter name to use.

This is repeated 3 times:

return "";

Should be string.Empty to better convey the intent. But let's look closer at the return mechanism:

if (results.Count == 0)
 return "";
foreach (SPListItem oListItem in results)
{
 return cleanValue(oListItem[returnColumnName].ToString());
}
return "";

The foreach loop isn't going to even iterate once if results is empty: that if(results.Count == 0) check is redundant, the return ""; after the loop is already covering for the "no results" case.

The catch(Exception ex) block is a problem though. You're not using ex, so you might as well have catch(Exception) there - but the real problem is that you're effectively thwarting what exceptions are for.

If I give that method string.Empty, or a null value for theList, or a column name that doesn't exist - I expect an exception to be thrown. Exceptions are good, exceptions are your friends: exceptions help you diagnose and remove bugs in the rest of your code - by swallowing all exceptions here, you make it harder to diagnose problems that might exist at a much higher level, e.g. your UI has a bad binding and consistently ends up calling GetValueFromList with a null parameter, but you have no way of telling if the "" return value is because there really isn't any such matching value, column or list, or because an ArgumentOutOfRangeException was swallowed there.

Answer 3

Borrowing from @Abbas last snippet:

var results = oList.GetItems(spQuery);
if (results.Count == 0)
 return "";
var item = results[0];
return CleanValue(item[returnColumnName].ToString());

If you are using C# 6 you can turn CleanValue into an extension and make it really nice looking:

static class Extensions
{
 public static string Clean(this object obj) 
 {
 ...obj.ToString and clean-up 
 }
}

then you could just to this:

return oList.GetItems(spQuery).SingleOrDefault()?.Clean() ?? string.Empty;

• Abbas: SharePoint 2010 relies on .NET 3.5 (SP1) and does not support .NET 4 or higher, so this will not work.