Refactoring Commerce catalog code, a story

It is not a secret that I am a fan of refactoring. Clean. shorter, simpler code is always better. It’s always a pleasure to delete some code while keeping all functionalities: less code means less possible bugs, and less places to change when you have to change.

However, while refactoring can bring a lot of enjoying to the one who actually does it, it’s very hard to share the experience: most of the cases it’s very specific and the problem itself is not that interesting to the outside world. This story is an exception because it might be helpful/useful for other Commerce developer.

I noticed that we have these two snippets, which basically save a model to a node/entry CommerceMediaCollection

public void SaveCatalogEntryItemAsset(ContentReference contentLink, ItemAsset asset)
        {
            var entry = _contentRepository.Get<EntryContentBase>(contentLink, new LoaderOptions() { LanguageLoaderOption.MasterLanguage() }).CreateWritableClone<EntryContentBase>();
            var entryAsset = entry.CommerceMediaCollection.FirstOrDefault(x => GetAssetKey(x.AssetLink) == asset.AssetKey && x.AssetType == asset.AssetType);
            if (entryAsset == null)
            {
                entryAsset = new CommerceMedia
                {
                    AssetLink = ConvertPermanentLinkToAssetLink(asset.AssetKey),
                    AssetType = asset.AssetType,
                    GroupName = asset.GroupName,
                    SortOrder = asset.SortOrder
                };
                entry.CommerceMediaCollection.Add(entryAsset);
            }
            else
            {
                entryAsset.GroupName = asset.GroupName;
                entryAsset.SortOrder = asset.SortOrder;
            }
entry.CommerceMediaCollection = new ItemCollection<CommerceMedia>(entry.CommerceMediaCollection.OrderBy(m => m.SortOrder));

_contentRepository.Save(
entry,SaveAction.Publish, AccessLevel.NoAccess);
}

and

        public void SaveCatalogNodeItemAsset(ContentReference contentLink, ItemAsset asset)
        {
            var node = _contentRepository.Get<NodeContent>(contentLink, new LoaderOptions() { LanguageLoaderOption.MasterLanguage() }).CreateWritableClone<NodeContent>();
            var nodeAsset = node.CommerceMediaCollection.FirstOrDefault(x => GetAssetKey(x.AssetLink) == asset.AssetKey && x.AssetType == asset.AssetType);
            if (nodeAsset == null)
            {
                nodeAsset = new CommerceMedia
                {
                    AssetLink = ConvertPermanentLinkToAssetLink(asset.AssetKey),
                    AssetType = asset.AssetType,
                    GroupName = asset.GroupName,
                    SortOrder = asset.SortOrder
                };
                node.CommerceMediaCollection.Add(nodeAsset);
            }
            else
            {
                nodeAsset.GroupName = asset.GroupName;
                nodeAsset.SortOrder = asset.SortOrder;
            }
node.CommerceMediaCollection = new ItemCollection<CommerceMedia>(node.CommerceMediaCollection.OrderBy(m => m.SortOrder));
_contentRepository.Save(node, SaveAction.Publish, AccessLevel.NoAccess);
}

Don’t they look the same? The only different is with the content type – one uses NodeContent and one uses EntryContentBase. We naturally want to combine those two methods into one to reduce code duplication, and it should start with trying to use their shared base class instead.

However, NodeContent and EntryContentBase ‘s shared base class is CatalogContentBase, which does not implement IAssetContainer , which has the CommerceMediaCollection we need.

My first attempt looks like this

        public void SaveCatalogItemAsset(IAssetContainer assetContainer, ItemAsset asset)
        {
            var writeable = ((IReadOnly) assetContainer).CreateWritableClone() as IAssetContainer;

            var nodeAsset = writeable.CommerceMediaCollection.FirstOrDefault(x => GetAssetKey(x.AssetLink) == asset.AssetKey && x.AssetType == asset.AssetType);
            if (nodeAsset == null)
            {
                nodeAsset = new CommerceMedia
                {
                    AssetLink = ConvertPermanentLinkToAssetLink(asset.AssetKey),
                    AssetType = asset.AssetType,
                    GroupName = asset.GroupName,
                    SortOrder = asset.SortOrder
                };
                writeable.CommerceMediaCollection.Add(nodeAsset);
            }
            else
            {
                nodeAsset.GroupName = asset.GroupName;
                nodeAsset.SortOrder = asset.SortOrder;
            }

            writeable.CommerceMediaCollection = new ItemCollection(writeable.CommerceMediaCollection.OrderBy(m => m.SortOrder));
            var saveAction = ((IVersionable) assetContainer).IsPendingPublish ? SaveAction.Save : SaveAction.Publish;
            _contentRepository.Save((IContent)writeable, saveAction, AccessLevel.NoAccess);
        }

It works, it reduces the code line by half, and because this method is “internal” we know for sure that the casts are safe and we will never run into exception. But still, it’s a bit ugly to look at.

As any decent developer I can’t stop thinking about it and can’t even sleep at night

Is there a way to make it better without all the cast.

Generic comes to rescue! With where statement we can define the type constraint with ease. And my second attempt look like this

public void SaveCatalogItemAsset(T assetContainer, ItemAsset asset) where T : CatalogContentBase, IAssetContainer
        {
            var writeable = assetContainer.CreateWritableClone() as T;

            var commerceMedia = writeable.CommerceMediaCollection.FirstOrDefault(x => GetAssetKey(x.AssetLink) == asset.AssetKey && x.AssetType == asset.AssetType);
            if (commerceMedia == null)
            {
                commerceMedia = new CommerceMedia
                {
                    AssetLink = ConvertPermanentLinkToAssetLink(asset.AssetKey),
                    AssetType = asset.AssetType,
                    GroupName = asset.GroupName,
                    SortOrder = asset.SortOrder
                };
                writeable.CommerceMediaCollection.Add(commerceMedia);
            }
            else
            {
                commerceMedia.GroupName = asset.GroupName;
                commerceMedia.SortOrder = asset.SortOrder;
            }

            writeable.CommerceMediaCollection = new ItemCollection(writeable.CommerceMediaCollection.OrderBy(m => m.SortOrder));
            var saveAction = assetContainer.IsPendingPublish ? SaveAction.Save : SaveAction.Publish;
            _contentRepository.Save(writeable, saveAction, AccessLevel.NoAccess);
        }

No more cast. Even better we now have compile time checks, so if your teammate tries to trick your code by calling a fake IAssetContainer implementation, their code will not even compile.

The same technique can be used to reduce code duplication and casting between types.

Now I can sleep in peace.