Uncategorized

Code reviews on Sitecore


Thomas Eldblom and I are reviewing most of the projects, we do in Pentia. The most of the things we comment on concerns C#, architecture, HTML, XSL or other general technologies, but some issues are related to the use of Sitecore. In this post I want to share the most common, most severe or most irritating issues/pitfalls I often stumble upon or look for, when doing a code review. I am glad to say, that I almost never stumble upon the most basic things, such as layouts specified directly on items, masters set on masters (SC5) etc.

·         Use of Sitecore.Context in logic

When having actual classes or general logic, I sometimes find a reference to the Sitecore.Context. Often it is a reference to Sitecore.Context.Item or Sitecore.Context.Database. This is rarely a good idea. The logic should be independent of the context it runs in. Instead you should pass in the objects you need as parameters. If you don’t do this, you’ll bind your logic to a certain context, making it unusable, if you want to use it without a context. Further logic that uses the context isn’t very testable.

 

·         Iterating over to many items

I don’t find this sort of issue very often, but it is pretty severe. The scenario is often that you have a list of all latest documents or a similar relational operation. You implement a simple XSL using “//item”. This works fine in development, but when the client starts importing or creating several thousand documents, the list doesn’t perform and brings the whole site down.

 

·         Too much logic in an XSL

Often you start out with a page type, which looks like a simple rendering. Therefore you start out creating an XSL. As you dig into the specs, you find out, that you need a lot of logic. You end up with a large amount of XSL extensions and a complex rendering. The result is a way to complex XSL, which is practically unreadable and impossible to maintain. When you have a lot of logic use a sublayout.

 

·         Reference to specific databases

Sometimes I find a reference to a specific database. For instance something like Sitecore.Configuration.Factory.GetDatabase(‘web’). Allthough it may be needed in some situations, it often gives some unintended sideeffects. Most of the time you want to operate on the context database, so you use the correct database specified in the site section in the web.config. A direct reference to the web database might cause preview functionality not to work as intended, and a direct reference to the master database might not work if you are using a staged environment.

 

·         Null reference

As Sitecore is so type weak and everything is an item, you don’t know what fields/properties the item has. No matter how experienced the developer is, I often find the use of a field of some sort, where it hasn’t been validated whether the field is null or not, resulting in a null reference exception if the input is incorrect.

 

·         Overriding standard Sitecore functionality

Sometimes a developer isn’t satisfied with the way Sitecore implements something – for instance the indexing, the site resolver, the domain etc. As Sitecore allows you to override almost everything, the developer thinks… Hey, I’ll just override the domain doing my own implementation. Although this might be necessary, it should be considered carefully. I have too many times stumbled into a problem with a custom implementation of standard functionality, either because it may be more complex than first assumed, the next developer on the project isn’t aware of it or last but not least an upgrade of Sitecore changes the requirements to the functionality.

 

I am hoping you want to share some of your experiences as well – telling what Sitecore pitfalls you often stumble upon or find very irritating.

Standard

12 thoughts on “Code reviews on Sitecore

  1. Lars Nielsen says:

    Hi Jens,

    This post expresses exactly my thoughts on most common mistakes. While some of your issues refers to generic programming techniques (such as transferring parameters instead of methods picking up values), some of your issues refers to breach of best practices.

    The most common mistakes, in regards to best practices, is overcomplicated XSLT’s and developers overriding standard functionality.

    In general, when programming a web site, you should not need to write more than 5-6 XSLT extension methods. If you do, you’re probably on the wrong track. XSLT files should almost always rest alone (using only the standard Sitecore extensions) and solve simple logic only. Otherwise, they become cumbersome, hard to read and very often very slow. Too often, I see XSLT controls resolving very complex menu structures or multi-dimensional lookup, and while almost everything is feasible in XSLT, it is a functional programming language, with the limitations and issues such a language raises.
    Another wise approach is with regular intervals to check up on standard functions in the included helper classes. Too often developers has built new functions solving exactly what standard extensions do.

    Your issue with overriding standard Sitecore functionality is also a very common breach of best practices. If any way possible, developers should try to build on top of standard functionality, – perhaps extending pipelines (remember to use web.config include files instead of modifying the web.config).

    While it is possible to override standard functionality, this may cause some grief when you wish to upgrade your Sitecore installation. And, – as you correctly observe, very often whoever overrode the function has very little idea of the relationship to other Sitecore classes and objects and the fact that such modifications may cause other methods to malfunction.

  2. Andreas says:

    Good write up! Always interesting to read about other developers experiences. I do have one question though. Could you clarify on the first topic “Use of Sitecore.Context in logic”? What would you use instead of Sitecore.Context? How else would you get the current item in code behind? Or am I missing your point? 🙂

  3. Pingback: Sitecore Fetch Squad » Blog Archive » Code reviews on Sitecore

  4. Jens Mikkelsen says:

    @Andreas.

    Well, it is ok to use the context in your presentation layer (eg. codebehind). But when you have a class, that has nothing to do with the presentation layer (for instance a Manager class), you should pass in the current item as a parameter and not refer to the Sitecore.Context.
    The same actually goes for HttpContext or any other context.

  5. Excellent writeup. If I would add anything it would be:

    Make sure similar fields are actually inheriting from common templates. Often have I found fields like “Menu Title”, “MenuTitle”, “MenuText” sitting in various templates – all obviously being one and the same. It creates such a mess in the renderings and not only that; it becomes almost impossible to refactor the template design afterwards. If anyone doesn’t know, Sitecore ultimately refers to fields via their guid, so creating a new field called “Menu Title” on a base template and then inheriting from it will still cost you all the data of the field.

    As for rewriting Sitecore, it always raises flags with me as well. Red flags, not even yellow ones. Reminds me of “I guess that would work, too” from http://thedailywtf.com/Articles/A-Problem-at-the-Personal-Level–More.aspx.

  6. rasmus says:

    @Andreas – It has something to do with dependencies. When directly relying on static available objects like Sitecore.Context the dependency of that specific object is very high. This makes automated tests more difficult as an example of a drawback. It can for example be solved by dependency injection as Jens said. You should also read up on static gateways, inversion of control etc. This all has to do with dependencies.

  7. This one is broader, but especially important inside Sitecore: not understanding Sitecore (and maybe CMS in general) patterns and practices.

    Not getting the content hierarchy, Sitecorish way of everything being an item, etc. This often leads to weird designs that just feel weird.

  8. Following on from what Alexey said, how you actually structure your data items in Sitecore plays an important role on data querying and retrieval.
    Also, people need to undertand that, in Sitecore, we deal with items…not pages!! 😛

  9. Richard Dias says:

    @Mark, Please clarify.
    You say “Make sure similar fields are actually inheriting from common templates.” and I agree. Then you go on to say “creating a new field called “Menu Title” on a base template and then inheriting from it will still cost you all the data of the field.” Which implies that we shouldnt use a base template to hold common fields. These two statements seem contradictory to me, please emphasise the difference between Common template and Base template? I am assuming that “Menu Title” needs to contain different text for each item it is being used on and is not ‘common’ to the whole site.

  10. Nick Tzanev says:

    Thanks for the write up Jens. I poited the rest of our SC developers this way. We are learning!

  11. Pingback: Sitecore code reviews « What 3Sixty Knows…

  12. Pingback: 5 worst code smells in Sitecore | cmsnewstoday.com

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