.NET, Architecture, Sitecore, Uncategorized

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?

Standard

8 thoughts on “5 worst code smells in Sitecore

  1. I often notice something smelly, when I see Sitecore field values being picked up in the code, but not cast to it’s corresponding type.

    Could be something like this:

    Field eventDate = myItem[“eventdate”];

    Which is usually followed by all sorts of string parsing functionality to try and get the ISO date format that Sitecore uses internally into a .NET DateTime format.

    The same goes for all of Sitecore’s field types, MultiList fields and so on.

    In my experience, developers lacking this basic understanding of the Sitecore API usually also leaves other code smells elsewhere in the solution, and should be looked closer into.

  2. Curious about proposed alternatives to the descendants axis. I personally have switched to querying Lucene indexes when I need to sift through that much data, but is there another generally recommended approach I should think about?

    • Using the Lucene index is the best replacement for the descendant axis. Previous versions of Sitecore (5.x and some versions of 6) had issues with the Lucene index being cleared or not updated, but these issues have been resolved in the latest versions.
      The lucene index is still usable from an XSLT extension (if you prefer XSLT’s) but it’s even easier to use them from a asp:Repeater or asp:ListView.

  3. Steve Green says:

    My top 5 stinks would be:

    1) Wrong tool for the wrong job, like you said complex XSLT or inversely simple .Net renderings.
    2) Poor IA, no template inheritance, too few templates, no logical separation of page items from data items.
    3) Field data not been output via field renderer, most common variation on this theme is requesting the data from the database and then dumping into a literal control.
    4) Incorrect use of paths and guids, misunderstanding of when to use which one.
    5) Large datasets of items needlessly stored in memory, more than once I’ve seen almost the entire content tree pulled into a monster List()

    Hmmm… I’m sure if I think about it more this list would change so maybe not top 5, just 5 particularly bad stinks 😉

  4. John says:

    There is one conclusion to this: all sitecore solutions smell badly. For the simple reason that no sitecore development company is using only top notch experienced sitecore developers. In fact most of them are using beginners and mediocre developers led by one senior. So basically all sitecore based sites are crapy.

    • Eldblom says:

      John,

      Interesting comment – however I choose to interpret your viewpoint as a result of a narrow horizon.
      I do agree that Sitecore in some markets is a new and upcoming product, and therefore there is a lack of top notch qualified Sitecore partners. Also some Sitecore partners does not deliver the quality which could be expected – let this be a appeal to Sitecore to be more critical in their partner program. But this said, when doing our professional services in Denmark and around the world, we meet a lot of very qualified Sitecore developers and implementation partners which focus on Sitecore as one of their primary products. Therefore I do not regard your comment as valid on Sitecore, but merely your (somewhat narrowminded) view on the whole software development industry.

      Finally, the general advice for all clients looking to incorporate a product in their company is to be critical about your implementation partner. Just because they claim to be able to work with a product – and even if they have a track record – remember to check up on references. Also consider hiring a third-party reviewer in the implementation process – like Pentia Professional Services 🙂 Remember that just because the product is awesome doesn’t mean that your implementation will be.

  5. Nick Smith says:

    How about the documentation … everything split into 30+ pdf files – I have my daily hour’s trawl through pdf after pdf, trying to find answers to some Sitecore question.
    If I didn’t know better, I might suspect Sitecore was trying to prevent anyone from finding anything out 🙂

    Have a Wiki – it’s not difficult.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s