RWE: Sample Code Review

What was an issue?

How does code look like?

How to improve existing code?

Problem 1: Performing Unnecessary Actions

Let's focus on the PopulateNodes(int locationID) method first. As you can see for every location (basically row record) in the _lmds.LocationDataTable.Rows collection the custom code calls DeleteSchedules(int locationID, BranchManagerDataSet bm) method (line no. 187). Inside this method our code retrieves the collection of 'Programs' nodes existing in the content tree by using the TreeProvide.SelectNodes() method (line no. 216).

As the next step code loops through the retrieved program nodes and for every node it:

FIX 1: There is actually no reason to call the SelectSingleNode() method to get TreeNode object. All the information about the node is already present in the DataRow dr (line no. 221). Thereby, instead of calling the SelectSingleNode() we can create new instance of the TreeNode object using the new TreeNode(dr, "<document type name>", treeProvider) constructor where we simply pass the DataRow as parameter. The small change suggested above saves 1 query per every program node per every location. If there would be 100 locations with let's say only 10 program nodes per location we would have just saved 1000 quite expensive queries.

Problem 2: Duplicate Actions 

There is another issue with the DeleteSchedules(int locationID, BranchManagerDataSet bm) method. You can see code calls the DocumentHelper.DeleteDocument() method first and then the node.Delete() method right after that (lines no. 229 and 230). The DocumentHelper.DeleteDocument() should be used for documents under the workflow. Hence it is absolutely correct to call DeleteDocument() as long as we are dealing with versioned documents. The node.Delete() method on the other hand should be used only for documents without workflow because unlike DocumentHelper.DeleteDocument() the node.Delete() does not care about multiple versions of the document and therefore is not suitable in our scenario.

FIX 2: There is no reason to call both the DocumentHelper.DeleteDocument() and node.Delete() method at the same place. Actually, it is highly inefficient because the node.Delete() executes additional query for every program node for every location after the document is already wiped off by DeleteDocument() called earlier.

We can again save hundreds or even thousands of queries just by removing the node.Delete() call.

NOTE: The same fix needs to be applied to the code few lines later when working through bucket of unprocessed documents (lines no. 235 - 258).

Problem 3: No Data Caching & Inefficient Insert Operation

Inside the AddNewSchedules(int locationID, BranchManagerDataSet bm) method custom code calls the GetWebTimeSFYPDataByLocationID(int locationID) and GetWebItemItemCrossReferenceByLocationID(int locationID) methods (lines no. 319 and 320) that retrieve data from database executing 2 queries in total for every location. Further in the same method code also retrieves all the location's program nodes (line no. 330). The code then loops through the import data (source data for new document sent from external service) and calls the CreateNode(string parentpath, CMS.TreeEngine.TreeProvider tree, WebTimeSFYPManagerDataSet.SOMETHING_WebTimeSFYPDataTableRow dsrow, string webservicelocationID) method to insert new document in the content tree (lines no. 350, 363 and 377).

In the CreateNode() method custom code uses TreeProvider.SelectSingleNode() (line no. 399) to get the parent node required whenever we are inserting a new document. The CreateNode() method accepts parent NodeAliasPath as first parameter. This parent NodeAliasPath comes from the DataRow that contains full information on the parent node - like NodeID for example.

At the bottom of the CreateNode() method code uses the node.Insert(parentNodeID) (line no. 436) to finally insert a new document in the content tree.

FIX 3: As you can see most of the custom methods (lines no. 68 - 129) pulling out additional information related to the document created (e.g. location details) execute single query for every location being processed. The results of those queries are not cached at all even though caching is perfect fit in scenarios like this. We should therefore implement caching mechanism for the location data obtained from database to streamline data retrieval and processing. The new API used to simplify caching implementations is explained here.

Ideally we should go even step further and instead of querying DB for every location separately we could retrieve details for all locations within the single request, cache the whole result set and later filter out records from the DataSet stored in a memory rather than accessing DB over and over again.

FIX 4: : Instead of selecting the parent node object using the TreeProvider.SelectSingleNode() (line no. 399) using the parent NodeAliasPath we want to pass the parent NodeID as input parameter for the CreateNode() method. The parent NodeID is available in the same DataRow as NodeAliasPath in the original code. This would save us another call to the database curently fired by TreeProvider.SelectSingleNode() (line no. 399). If you realize that every call to the SelectSingleNode() executes additional query to get parent information already available we can eliminate another query for every document created.

FIX 5: To further optimize the code inserting new documents we want to call the node.Insert(parentNodeObj) instead of node.Insert(parentNodeID) (line no. 436). If you call the node.Insert() method with the parent NodeID as parameter the Insert() method will still internally call the SelectSingleNode() to get the parent node object. Let's call the Insert() with the parent node object directly and we get rid of another expensive query. As I have already mentioned to create new instance of parent TreeNode object we just need to use the new TreeNode() constructor and pass the parent DataRow as one of its parameters.

Summary

As you could see when it comes to the performance issues with your custom code is more-or-less about efficiency of your code. And by most part the efficiency goes hand in hand with costs of data access and transfer. That being said your attention should be aimed on how much data you retrieve from data base, what is reusability level of such data and whether or not the same data are not already available at different source.

Before you go...

I would love to hear some feedback from you on whether you find posts like this useful, what I should change or add to better serve their purpose and so on. I can imagine we could do some series from articles on code review/best practices topics, however, I need to know if it is something you may be interested in. Please use comments below to pass the message.

Thank you!