In the previous post, I introduced how to use MSOCAF to check the custom solution. There are a lot of issues after checked, and now I will focus on the issue that happens frequently. Read this article to improve your habits, change the mind and build a good product.
Case 1: use SPListItem.Update
Even though the Update() method is found in SPListItem and use to update any field in item. But it’s only available when we have separate updating on each item. Don’t use this method in loop statement, especially for large list. See the code
1: public void UpdateAll(SPListItemCollection items)
2: {
3: foreach (SPListItem itm in items)
4: {
5: itm["Count"] = 0;
6: itm.Update();
7: }
8: }
9:
It should be changed to use update command and execute by SPWeb object. Best practice code is as below:
1: public void UpdateAll(SPListItemCollection items)
2: {
3: StringBuilder builder = new StringBuilder();
4:
5: builder.Append("<?xml version=\"1.0\" encoding=\"UTF-8\"?><ows:Batch OnError=\"Return\">");
6:
7: foreach (SPListItem itm in items)
8: {
9:
10: string methodFormat = "<Method ID=\"{0}\">" +
11: "<SetList>{1}</SetList>" +
12: "<SetVar Name=\"Cmd\">Update</SetVar>" +
13: "<SetVar Name=\"ID\">{2}</SetVar>" +
14: "<SetVar Name=\"urn:schemas-microsoft-com:office:office#Count\">{3}</SetVar>" +
15: "</Method>";
16:
17: builder.AppendFormat(methodFormat, itm["ID"], list.ID, itm["ID"], 0);
18: }
19:
20: builder.Append("</ows:Batch>");
21:
22: web.ProcessBatchData(builder.ToString());
23: }
24:
Because the Update() method is called in the loop, so that’s not best practice to improve performance. It is reported under custom rules of Microsoft Online when we run the MOSCAF checking. That is to be able to understand in the context of cloud environment. However, if there is a large list, please don’t add many thousands items into an update string, just less than 2000 that’s fine. To read more the performance when item is updated, please read here http://msdn.microsoft.com/en-us/library/cc404818(v=office.12).aspx
Case 2: SPListItemCollection\GetItemByID inside loop
See the code below:
1: SPList list = SPContext.Current.Web.Lists["Custom"];
2:
3: for (int i = 0; i < 10; i++)
4: {
5: SPListItem item = list.GetItemById(itm[i]);
6: //Doing something
7: }
8:
Almost developers think about the code above is very normal and nothing wrong. Of course that’s not, but it isn’t the best practice too. Because when we call list.GetItemById method that means the collection of SPListItem is retrieved and call again in the next index.
The code is changed:
1: SPList list = SPContext.Current.Web.Lists["Custom"];
2:
3: for (int i = 0; i < 10; i++)
4: {
5:
6: SPQuery query = new SPQuery();
7: query.RowLimit = 1;
8: query.Query = “<where><Eq><FieldRef Name=’ID’/><Value Type=’Number’>” + itm[i] + “</Value></Eq></Where>”;
9:
10: SPListItemCollection collection = list.GetItems(query);
11:
12: SPListItem item = collection[0];
13:
14: item["Count"] = 0;
15:
16: item.Update();
17: }
However, when we call the Update() method in loop statement we will get the error as case 1, we must combine case 1 and case 2 to fix together.
Case 3: Use SPList.Items
See the code:
1: public void DeleteSchemaItem(string url, string listName, int idItem)
2: {
3:
4: using (SPSite site = new SPSite(url))
5: {
6: site.CatchAccessDeniedException = false;
7:
8: using (SPWeb web = site.OpenWeb())
9: {
10: SPList list = web.Lists[listName];
11:
12: list.Items.DeleteItemById(idItem);
13: }
14: }
15: }
16:
The code above seem to work well for the functioning, however, it is only for very small list (a very little items in list). And with the large list, the code above is not good performance and need to be improved. Because when we call list.Items that means the collection of SPListItem is retrieved and after delete an item, this collection was re-modified.
The code is changed:
1: public void DeleteSchemaItem(string url, string listName, int idItem)
2: {
3: using (SPSite site = new SPSite(url))
4: {
5: site.CatchAccessDeniedException = false;
6: using (SPWeb web = site.OpenWeb())
7: {
8: SPList list = web.Lists[listName];
9: SPListItem item = list.GetItemById(idItem);
10: item.Delete();
11: }
12: }
13: }
14:
The code seems good with performance because it doesn’t reach to the very large collection at least.
Case 4: SPQuery object without RowLimit property
We often forget the most important property of SPQuery object, that is the RowLimit propery. If this property is not set, what is the default value? And as we know this value is accepted from range [1…2000]
It’s so excited when I work on this object, because most of us forgot the work as design of this object and thought to leave RowLimit property to wish getting all data when they match with the query CAML. And the code looks like:
1: SPQuery query = new SPQuery();
2:
3: query.Query = "<Where><Gt><FieldRef Name='ID'/><Value Type='Number'>1</Value></Gt></Where>";
4:
5: SPListItemCollection col = GetData(query, list);
The code will return all items in the list; however it will throw exception if the number of list item is greater than default value of Throttling in Central Administration configuration. The default value of Throttling is 5000 and this value is able to be overwritten by the code.
1: SPSecurity.RunWithElevatedPrivileges(delegate()
2: {
3: SPQuery query = new SPQuery();
4: query.Query = "<Where><Gt><FieldRef Name='ID'/><Value Type='Number'>1</Value></Gt></Where>";
5: query.QueryThrottleMode = SPQueryThrottleOption.Override;
6: query.RowLimit = 5500;
7:
8: SPListItemCollection col = GetData(query, list);
9: });
10:
We must run the code under Full Control account, so the code is reverted to web application pool account to apply the overwrite throttling.
But with the MSOCAF, the RowLimit item is only valid in range [1...2000] unless you will get the error issue for performance. The important thing is the code above is a weakness security. For many purpose, we need to get all data with good performance and security but not violate the rule of MSOCAF.
The code is changed again:
1: SPQuery query = new SPQuery();
2:
3: query.RowLimit = 2000;
4: query.Query = "<Where><Gt><FieldRef Name='ID'/><Value Type='Number'>1</Value></Gt></Where>";
5:
6: do
7: {
8: SPListItemCollection col = GetData(query, list);
9: query.ListItemCollectionPosition = col.ListItemCollectionPosition;
10: }
11:
12: while (query.ListItemCollectionPosition != null);
13:
Case 5: Types that own disposable fields should be disposable
That means if we have implemented the class with some methods or properties return the object that implemented IDisposable interface, we also must inherit IDisposable and implement the code to dispose object or dispose object manually. In case, the class has shipped we can create a new method to manage the disposing object.
See the code
1: public class ItemDetail : ITemplate
2: {
3:
4: CheckBox chk;
5:
6: public void InstantiateIn(Control container)
7: {
8: chk = new CheckBox();
9: chk.ID = "chk";
10:
11: container.Controls.Add(chk);
12: }
13: }
14:
And create an object in CreateChildControl method of web part:
1: protected override void CreateChildControls()
2: {
3:
4: SPGridView grid = new SPGridView();
5: grid.AutoGenerateColumns = false;
6:
7: TemplateField tplFeild = new TemplateField();
8: tplFeild.HeaderTemplate = new ItemDetail();
9:
10: grid.Columns.Add(tplFeild);
11: }
12:
CheckBox is ASP.NET control and have implemented IDisposable interface, but we don’t care about disposing control when we add it into ASPX page because they are disposed when the page is disposed. And regarding code above, when the page is diposed there is only grid object is disposed and tplField.HeaderTemplate didn’t disposed, but chk object need to be disposed, so we must dispose manually.
See the code is changed:
1: public class ItemDetail : ITemplate, IDisposable
2: {
3:
4: CheckBox chk;
5:
6: private bool disposed = false;
7:
8: public void InstantiateIn(Control container)
9: {
10: chk = new CheckBox();
11: chk.ID = "chk";
12: container.Controls.Add(chk);
13: }
14:
15: public void Dispose()
16: {
17: Dispose(true);
18: GC.SuppressFinalize(this);
19: }
20:
21: protected virtual void Dispose(bool disposing)
22: {
23: if (!this.disposed)
24: {
25: if (disposing)
26: {
27: if (chk != null) chk.Dispose();
28: }
29: disposed = true;
30: }
31: }
32:
33: ~ItemDetail()
34: {
35: Dispose(false);
36: }
37: }
38:
We can use the keyword ‘using’ to dispose object automatically. There are also some objects do not need to dispose and we know that when method dispose is called, so it does nothing but we still use ‘using’ as a habit. Read more here http://weblogs.asp.net/pwilson/archive/2004/02/20/77435.aspx and http://msdn.microsoft.com/library/ms182172(VS.90).aspx
Case 6: Do not cast unnecessarily
There are many times we cast the variable to the same type and think about that as a safety. For example:
XmlElement tempElement;
1: foreach (XmlNode node in nodes)
2: {
3: string realDisplayName = ((XmlElement)node).Attributes["DisplayName"].InnerText;
4: }
5:
It is so clearly that variable named ‘node’ is an element of XML document, even though it is retrieved from XmlNode collection (XMLNodeList) but it’s still an element. Thus, the casting value of node variable to XmlElement is not necessary.
Another example in SharePoint:
1: SPField field = list.Fields.GetFieldByInternalName("OurColumnsName");
2:
3: if (field is SPFieldDateTime)
4: {
5: SPFieldDateTime fDateTime = field as SPFieldDateTime;
6: //DO SOMETHING
7: }
8:
The ‘is’ statement is casted object and the assignment object fDateTime with ‘as’ is duplicated casting. So we must change the code, please read this document to know how to change the code to match the rule of MSOCAF. Here is the link: http://msdn.microsoft.com/library/ms182271(VS.90).aspx
