How would you improve this code?

Vote:
 

Here's the scenario; we use a page's teaser image to populate meta tags such as og:image or twitter:image so that if someone were to try and share one of our pages on social media, that is the image which is auto-populated in whatever sharing method / tool they are using. We love a good SVG, but as it turns out, some of the social sharing tools don't. Facebook for example won't render SVGs and so that image is either left blank when someone shares or that sharer has to be diligent enough to manually select an image - doesn't happen that often.

There were a few avenues we could explore as solutions to this. We looked at just having one static image for sharing - company logo perhaps, which seemed a bit boring. Or possibly a suite of static images which could be swapped based on page type. That seemed a bit more flexible and dynamic, but if we wanted dynamic then why not do something with the SVG itself? Like converting the SVG to a PNG on the fly! We looked around online, but couldn't see anyone who'd already developed this before and it sounded like the most fun approach so that's what we went with.

The last thing to note is that we're running EPiServer 11 in DXC which means Azure Blobs!

In _Root.cshtml we had code that looked like this:

<meta name="twitter:image" content="@Model.CurrentPage.TeaserImage.ContentExternalUrl(System.Globalization.CultureInfo.InvariantCulture, true)" />

We wanted to replace the call to ContentExternalUrl() with some helper method which would check for the presence of an SVG and convert that to a suitably sized PNG and deliver that to the browser. Thinking ahead, we expected that conversion process to impact server performance to some degree so we only wanted to do the work when needed. With that in mind we wanted the helper method to check to see if we'd already done the conversion work before and deliver that image instead of doing it again and again.

We looked around online and decided that the package Svg v2.4.2 could do what we wanted.

I'll paste the code below which is reasonably commented, but the _Root.cshtml now contains:

<meta name="twitter:image" content="@SocialImageSharingHelper.ConvertImage(Model.CurrentPage.TeaserImage, ".png", Model.CurrentPage.ContentLink).ContentExternalUrl(System.Globalization.CultureInfo.InvariantCulture, true)" />

and here is the helper:

public static class SocialImageSharingHelper
    {
        //Summary:
        //  This helper is intended to be used in _Root.cshtml
        //  When pages have a teaser image in SVG format they are not
        //  always shared through social media in a desired way as
        //  we set the teaser image to the meta property og-url
        //
        //Parameters:
        //  imageReference:
        //      The ContentReference of the image to be converted.
        //
        //  fileExtension:
        //      string value of the file type the image is to be converted to. Must include the .
        //
        //  imageFolderReference:
        //      The ContentReference to the folder the current image is held.
        //
        //Returns:
        //      The ContentReference to the converted image, if conversion has happened. Else the original ContentReference
        //
        //TODO convert this to a service and use Dependency Injection (construction)
        public static ContentReference ConvertImage(ContentReference imageReference, string fileExtension, ContentReference imageFolderReference)
        {
            var contentRepository = ServiceLocator.Current.GetInstance<IContentRepository>();
            var blobFactory = ServiceLocator.Current.GetInstance<IBlobFactory>();
            var contentAssetHelper = ServiceLocator.Current.GetInstance<ContentAssetHelper>();
            var contentLoader = ServiceLocator.Current.GetInstance<IContentLoader>();

            //get the current image
            ImageData currentImage = contentRepository.Get<ImageData>(imageReference);

            //get the path to the current image
            string currentImagePath = ServiceLocator.Current.GetInstance<UrlResolver>().GetVirtualPath(imageReference).GetUrl(true);

            //get the extension of the file
            string extension = Path.GetExtension(currentImagePath);

            //if the file isn't an SVG then we don't need to convert, return early with the original ContentReference
            if (extension != ".svg")
            {
                return imageReference;
            }

            //we've got this far so we know we want to convert, unless the work has already been done!
            //TODO: Check to see if converted file already exists and if so return that reference

            //GetBlob only works with Uri id, so we can't just use a file name.

            //explore the current page's assets folder (as this is where we would have stored any converted files)
            string theFileWeAreLookingFor = $"{Path.GetFileNameWithoutExtension(currentImagePath)}{fileExtension}";
            var folder = contentAssetHelper.GetAssetFolder(imageFolderReference);
            foreach (var item in contentLoader.GetChildren<MediaData>(folder.ContentLink))
            {
                //check if item is the converted file we were looking for
                if ( item.Name.Equals(theFileWeAreLookingFor))
                {
                    return item.ContentLink;
                }
            }

            SvgDocument svgDocument;
            try
            {
                //SVgDocument.Open() likes to read files on disk and as ours are in Azure they aren't on disk, so we have
                //to use the overload for Stream

                //use the BlobFactory to get the image as an array of bytes
                byte[] blobBytes = blobFactory.GetBlob(currentImage.BinaryData.ID).ReadAllBytes();

                //read the byte array into memory as a stream
                using (MemoryStream ms = new MemoryStream(blobBytes))
                {
                    svgDocument = SvgDocument.Open<SvgDocument>(ms);
                }
                
            }
            catch(FileNotFoundException exception)
            {
                //something went wrong
                Console.Write(exception);
                return imageReference;
            }

            //imageFile is the blob container, location and data etc...
            var imageFile = contentRepository.GetDefault<ImageFile>(contentAssetHelper.GetAssetFolder(imageFolderReference).ContentLink);

            //Render the SVG as Bitmap using .Draw()
            //passing in 0 as the width element to keep aspect ratio
            var svgFileContents = svgDocument.Draw(0, 100);

            //Convert the Bitmap rendering
            ImageConverter converter = new ImageConverter();
            byte[] data = (byte[])converter.ConvertTo(svgFileContents, typeof(byte[]));

            //Create a blog with the file extension and everything.
            var blob = blobFactory.CreateBlob(imageFile.BinaryDataContainer, fileExtension);

            //File the blob with byte array data
            using (var s = blob.OpenWrite())
            {
                var w = new StreamWriter(s);
                w.BaseStream.Write(data, 0, data.Length);
                w.Flush(); //flush the writer once we're done
            }

            //assign the converted blob data
            imageFile.BinaryData = blob;

            /* Set the name of the file */
            imageFile.Name = $"{Path.GetFileNameWithoutExtension(currentImagePath)}{fileExtension}";

            /*add alt text otherwise the SaveAction.Publish will error*/
            imageFile.Description = $"{currentImage.Name} - This image was autogenerated.";
            //imageFile.Description = $"{currentImage.Description} - This image was autogenerated.";

            /* Consider providing an async .Save as per DXC Training page 62*/
            ContentReference convertedImage = contentRepository.Save(imageFile, SaveAction.Publish, AccessLevel.NoAccess);

            return convertedImage;
        }
    }

Lot's of borrowed code up and typos in there which we've ungracefully smashed together until the errors went away and converted PNGs were delivered to the browser!

So, as the title says, how would you improve this code? Any thoughts or constructive critisisms are most welcome.

Alex

#203703
May 03, 2019 12:38
Vote:
 
string theFileWeAreLookingFor = $"{Path.GetFileNameWithoutExtension(currentImagePath)}{fileExtension}";
            var folder = contentAssetHelper.GetAssetFolder(imageFolderReference);
            foreach (var item in contentLoader.GetChildren<MediaData>(folder.ContentLink))
            {
                //check if item is the converted file we were looking for
                if ( item.Name.Equals(theFileWeAreLookingFor))
                {
                    return item.ContentLink;
                }
            }

GetChildren can be expensive to start with, especially if you have a lot of children. if you already have already construct the url to the image you are looking, then use IUrlResolver.Route(UrlBuilder, ContextMode) instead, which is much faster.

Some other minor issues but nothing critical. 

#203704
May 03, 2019 12:46
Vote:
 

Thanks Quan, really appreciate the input! I'll have a look into Route().

If you fancy expanding on the minor issues I'd love to learn.

#203718
May 03, 2019 16:16
Vote:
 

Mostly coding style. For example you can move ServiceLocator.Current.GetInstance<UrlResolver>(), and use IUrlResolver instead. I would also think the code to create the new image can be extracted to a separated method. Most of the comments can be removed because they don't contribute anything. You should not use  Console.Write(exception); but proper logging, etc etc. As I said nothing critical but for maintainablity it might be worth it. 

#203719
May 03, 2019 16:21
Vote:
 

again, thank you! Maintainability is definitely a worthy reason to make the adjustments. :)

#203721
May 03, 2019 16:44
Vote:
 

Alex,
Just a reminder:
If you believe that someone answered your question satisfactorily, please click the Mark as answer icon next to the reply.

#203727
May 03, 2019 17:11
Vote:
 

thanks for the reminder Bob, too hastey to rush off an attempt to implement Quan's suggestions!

#203728
May 03, 2019 17:15
Vote:
 

just my 2 cents..

1) I would definitely implement that one TODO task, otherwise - you are converting from SVG to requested file format on every page request :)

2) I would convert it from static method to instance one. might be wondering why? just to get rid of ServiceLocator code :) do I need to create new instance then everytime I need access to that class? no, you can use some dependency injection magic - either by using `Injected<T>` type (which uses ServiceLocator behind the scene, but looks a bit nicer) or for example you can request that class in controller constructor and use in "on the server" or finally you can use some black magic to automatically inject this instance (as it might be useful for other pages as well if that class contains more image related utility helper methods). this trick is available by changed base class of the Razor pages in web.config and do magic in that new base page class.

3) excplicit or implict variable declarations (for example `var` vs `ImageData`)

4) proper async might be a bit problematic in this context (I'm refering to comment to `contentRepository.Save()` line)

#203741
May 05, 2019 20:17
Vote:
 

thanks for your input Valdis. 

1) I'll certainly give the TODO a shot, in the meantime I'm trying to return as early as possible so as not to run the conversion if I don't need to.

2) This definitely highlights a big gap in my knowledge. I'm certainly not experienced enough yet to dabble in the dark arts ;) but I like the sound of removing / obsfuscating the ServiceLocator code away from here so I'll look into the Injected<T> approach.

3) Am I right in thinking that here you mean pick one or the other (explict or implicit) and stick with it and not to mix?

4) Problematic in that I need to deliver the page and if I have an async save method then my page could get delayed whilst waiting for the save to finish?

#203770
May 07, 2019 9:53
Vote:
 

1) it's good to return as early as possible and avoid wasting CO2 to compute something that's not needed

3) it depends on your company's coding style guidelines, but I would at least recommend to keep it consistent

4) that's why I wrote proper async. This is not yet .NET Core (where even form rendering in the view has to be async). In general - more I'm looking into the code, more I have temptation to recommend actually do all this stuff "upfront" and then just serve prepared content for the page. By "upfront" here I'm refering to way to subscribe for example on ContentUpdating event (or whatever it was named) and then do preparation of the data and save it as part of the content. Then in page view - you are just rendering required properties of the content.

#203772
May 07, 2019 10:01
Vote:
 

[UPDATE]

Well it's been a fun few days. I've taken on board as much of the suggestions I received here as I could, manically googled all sorts of things and learnt a ton.

@Quan and @Valdis, you guys rock, I hope I've done your suggestions justice ha ha. This is what I've arrived at:

[ServiceConfiguration(ServiceType = typeof(SocialImageService), Lifecycle = ServiceInstanceScope.Singleton)]
    public class SocialImageService
    {
        private readonly IContentRepository _contentRepository;
        private readonly IBlobFactory _blobFactory;
        private readonly ContentAssetHelper _contentAssetHelper;
        private readonly IUrlResolver _urlResolver;
        private string _currentImagePath;
        private ContentReference _existingMatch;

        public SocialImageService(IContentRepository contentRepository, IBlobFactory blobFactory, ContentAssetHelper contentAssetHelper, IUrlResolver urlResolver)
        {
            this._contentRepository = contentRepository;
            this._blobFactory = blobFactory;
            this._contentAssetHelper = contentAssetHelper;
            this._urlResolver = urlResolver;
        }

        //Summary:
        //  Service which will convert an SVG to a desired bitmap
        //
        //Parameters:
        //  imageReference:
        //      The ContentReference of the image to be converted.
        //
        //  fileExtension:
        //      string value of the file type the image is to be converted to. Must include the .
        //
        //Returns:
        //      The ContentReference to the converted image, if conversion has happened. Else the original ContentReference
        public virtual ContentReference Render(ContentReference imageReference, string fileExtension)
        {

            if (ConversionNeeded(imageReference))
            {
                return imageReference;
            }

            if (ConversionExists(fileExtension))
            {
                return _existingMatch;
            }

            return Convert(imageReference, fileExtension);            
        }

        //Summary:
        //  Checks if a ContentReference's file has the extension ".svg"
        //
        //Parameters:
        //  imageReference:
        //      The ContentReference of the image to be converted
        //
        //Returns:
        //  bool:
        //      true if it's an svg, otherwise false    
        private bool ConversionNeeded(ContentReference imageReference)
        {
            _currentImagePath = _urlResolver.GetUrl(imageReference);

            string extension = Path.GetExtension(_currentImagePath);

            return (extension != ".svg");
        }

        //Summary:
        //  Checks if a converted version of the file exists based on desired file extension
        //  also stores a reference to the existing file if one is found
        //
        //Parameters:
        //  fileExtension:
        //      A string indicating the file extension required
        //
        //Returns:
        //  bool:
        //      true if a match is found, otherwise false
        private bool ConversionExists(string fileExtension)
        {
            string theFileWeAreLookingFor = $"/siteassets/{Path.GetFileNameWithoutExtension(_currentImagePath)}{fileExtension}";
            UrlBuilder theUrlWeAreLookingFor = new UrlBuilder(theFileWeAreLookingFor);
            IContent match = _urlResolver.Route(theUrlWeAreLookingFor);

            if ( match != null )
            {
                _existingMatch = match.ContentLink;
                return true;
            }
            else
            {
                return false;
            }
        }

        //Summary:
        //  Converts svg to bitmap and saves the converted file
        //
        //Parameters:
        //  imageReference:
        //      The ContentReference of the image to be converted
        //
        //  fileExtension:
        //      A string indicating the file extension required
        //
        //Returns:
        //  ContentReference:
        //      a reference to the newly converted file
        private ContentReference Convert(ContentReference imageReference, string fileExtension)
        {
            ImageData currentImage = _contentRepository.Get<ImageData>(imageReference);
            SvgDocument svgDocument;
            try
            {
                byte[] blobBytes = _blobFactory.GetBlob(currentImage.BinaryData.ID).ReadAllBytes();

                using (MemoryStream ms = new MemoryStream(blobBytes))
                {
                    svgDocument = SvgDocument.Open<SvgDocument>(ms);
                }

            }
            catch (FileNotFoundException exception)
            {
                ILog log = LogManager.GetLogger(typeof(SocialImageService));
                log.Error(exception);
                return imageReference;
            }

            ImageFile imageFile = _contentRepository.GetDefault<ImageFile>(SiteDefinition.Current.SiteAssetsRoot);

            Bitmap svgFileContents = svgDocument.Draw(0, 100);

            ImageConverter converter = new ImageConverter();
            byte[] data = (byte[])converter.ConvertTo(svgFileContents, typeof(byte[]));

            Blob blob = _blobFactory.CreateBlob(imageFile.BinaryDataContainer, fileExtension);

            using (var s = blob.OpenWrite())
            {
                StreamWriter w = new StreamWriter(s);
                w.BaseStream.Write(data, 0, data.Length);
                w.Flush();
            }

            imageFile.BinaryData = blob;

            imageFile.Name = $"{Path.GetFileNameWithoutExtension(_currentImagePath)}{fileExtension}";

            imageFile.Description = $"{currentImage.Name} - This image was autogenerated.";

            ContentReference convertedImage = _contentRepository.Save(imageFile, SaveAction.Publish, AccessLevel.NoAccess);

            return convertedImage;
        }
    }

and in order to gain access to this service from within _Root.cshtml, I've added a field to my LayoutModel.cs

public SocialImageService SocialImageService { get; set; }

and hooked that up to my PageViewContextFactory via a private field and also in it's constructor which then passes that through to the LayoutModel when it's created. 

I think my Convert() could probably be refactored some more, possibly splitting the saving of the file out. I'm also not particularly thrilled with using "/siteassets/" in ConversionExists(), but I struggled accessing the contentassets folder for any given page as I lack knowledge about folders and contentassetsfolders

I've tested this as best as I can locally and now it's off to integration and onwards to UAT!

#203912
May 10, 2019 17:20
Vote:
 

+1, you learn fast :)

#203928
May 12, 2019 17:50
Vote:
 

just my last 2 cents - I would avoid comments on top of methods with //. try typing tripple-slash (///) - it will generate XML docs for the  method that potentially could be used later for system documentation generation. also, if you are really into the documenting your code (which is very welcome) - you can try to install GhostDoc, it helps you to be more lazy (which is also welcome) ;) 

#203929
May 12, 2019 17:52