5 worst code smells in Sitecore

Last year I wrote a blog post on doing code reviews on Sitecore. While dealing with conceptual issues it doesn’t really say what to look for in code, when evaluating the quality or to evaluate if any wrong decisions have been made when developing the solution.  Therefore I wanted to wright a post about the top 5 code smells I look for, when I am trying to evaluate or trouble shoot a Sitecore solution. These code smells often indicate that the developers have gone down the wrong road and there is something fundamentally wrong in the architecture.

So here goes:

1) Using the descendant axes in xpath expressions:

This one I especially look for, when I am looking at a site which doesn’t perform. It may be used in some cases, but very often I find that a developer has used this to generate relations between content and thereby iterating over the complete content tree. This works fine in development where there are a few hundred content items. However in production where thousands of items are created this simply breaks the performance of the site – especially after publishing which clears the HTML cache.

The descendants can be accessed in multiple ways. In XSLT it normally looks like this:
<xsl:for-each select=”$sc_currentitem/descendant-or-self::item”>
</xsl:for-each>Or like this:
<xsl:for-each select=”$sc_currentitem//item”>
</xsl:for-each>

In C# using the Sitecore API it is most often accessed like this:

item.Axes.GetDescendants()

2) XSLT Import:

This is related to a point in the post about code reviews on Sitecore, where I argue that overcomplicated functionality and logic in XSLTs, is a problem. If you need to do an import of another XSLT in your rendering, you probably do it to call a method or in another way try and reuse functionality. If you want to do this, don’t use XSLTs! XSLTs should only – if you really want to use them at all – hold simple presentations.
XSLTs can be imported like this:

<xsl:import href=”YourOtherXslt.xslt” />

3) Explicitly accessing the master database

Although this can be needed in some cases and especially in scheduled tasks or other backend functionality, most often it is because the developer haven’t thought the issue through or if they have made a wrong decision of how data entered by the user should be stored.
The master database is most often used like this in c#

Database masterDatabase = Database.GetDatabase(“master”);

If you see a call like this, your solution is most likely not ready for staging, where there is no access to the master database from the content delivery server. You should generally use something like this:

Database database = Sitecore.Context.Database ?? Sitecore.Context.ContentDatabase;

This means you operate on the context database and on the content delivery databases this will be the web database and in preview you will use the master database, which is probably what you want.
If you need to write something to the master database, you first need to decide if this is a good idea at all. If you write to the master database from your frontend, you make yourself vulnerable for malicious attack, as people from the outside can enter data into the master database. Normally we handle user entered data in a custom database or parse all user created content to ensure that the master database won’t be flooded with data.
If you really need to access the master database, you should create a service that handles access to the master database, so that your solution is ready for staged environments.

4) XSLT extension over 100 lines of code:

This point is closely related to point number 2, but I really see a lot of issues with XSLTs which have been used wrong and this creates a lot of issues later on in the project lifecycle. If you have an XSLT extension with over a hundred lines of code, chances are, that the developer made a wrong decision when creating the XSLT; instead he/she should have created a sublayout.
XSLT extensions are functional programming and you should use an object oriented approach, so my advice is not to use XSLTs at all, unless you are doing something really simple. Otherwise your solution will become harder and harder to maintain, as utility methods like the XSLT extensions are very hard to understand and couplings in the renderings themself are difficult to map.
You can find all registered XSLT extensions in the web.config under xslExtension and it looks something like this:

<xslExtensions>
  <extension mode=”on” type=”MyNamespace.XslHelper, MyAssembly” namespace=”http://www.example.net/helper” singleInstance=”true”/>

5) Clearing of cache

This may seem obvious to some, but I have seen surprisingly many methods, which cleared the entire Sitecore cache including the data and item cache. If you see this in your code and it is needed, chances are, that you have an architectural issue in your solution. It should most rarely be needed to clear the cache, unless it is after a publish and Sitecore handles this automatically. I have heard many reasons for clearing the cache including an issue, where the developer argued that it was necessary, as he wrote directly to the Sitecore database with a SQL statement, and then Sitecores cache wasn’t updated.
If you experience something like this, the issue is of cause not the cache, but that you don’t use the Sitecore API to read and write data from the database.
The caching is most often cleared like this:

CacheManager.ClearAllCaches();

This is just some of the things I have seen many times, but I know there are many other bad code smells when developing Sitecore solution. Do you have any other code smells to share?