11
\$\begingroup\$

I'm just starting to develop more in C# after being mainly a VB.NET developer and was looking for someone to critique my implementation of a NewtonSoft Json.Net serialiser.

Can you provide some feedback on the following points:

  1. Is this a good way to build the class (using Unity)?
  2. Is it acceptable to be throwing an exception from the constructor?
  3. Is the Async/Await implementation correct?

Interface

using System.Threading.Tasks;

namespace Helper.Core.Serialisation
{
    public interface ISerialiser
    {
        /// <summary>
        /// Serialise the passed in object with the Json.Net serialiser
        /// </summary>
        /// <typeparam name="T">Generic type of the serialised object</typeparam>
        /// <param name="serialseObject">The object to be serialised</param>
        /// <returns>A serialised Json string</returns>
        Task<string> SerialiseAsync<T>(T serialseObject);

        /// <summary>
        /// Serialise the passed in object with the Json.Net serialiser and compress the string using the IStreamCompression implementation
        /// </summary>
        /// <typeparam name="T">Generic type of the serialised object</typeparam>
        /// <param name="serialseObject">The object to be serialised</param>
        /// <returns>A compressed byte array of the serialised object</returns>
        Task<byte[]> SerailseAndCompressAsync<T>(T serialseObject);

        /// <summary>
        /// Deserialise the Json string into the generic object
        /// </summary>
        /// <typeparam name="T">Generic type of the serialised object</typeparam>
        /// <param name="serialseObject">The object to be serialised</param>
        /// <returns>A deserialsied object of type T</returns>
        Task<T> DeserialiseAsync<T>(string serialsedString);

        /// <summary>
        /// Uncompress and deserialise the Json string into the generic object
        /// </summary>
        /// <typeparam name="T">Generic type of the serialised object</typeparam>
        /// <param name="serialed">The object to be serialised</param>
        /// <returns>An uncompressed & deserialsied object of type T</returns>
        Task<T> DeserialseAndUnCompressAsync<T>(byte[] serialed);
    }
}

Implementation

using System;
using System.Threading.Tasks;
using Helper.Core.Compression;
using Helper.Core.Logging;
using Microsoft.Practices.Unity;
using Newtonsoft.Json;

namespace Helper.Core.Serialisation
{
    /// <summary>
    /// Json.Net implementaiton of the ISerialiser interface
    /// </summary>
    internal class JsonSerialiser : ISerialiser
    {
        private readonly IStreamCompression _streamCompressor;
        private readonly ILogger _logger;

        /// <summary>
        /// Creates a new instance of the Json.Net Serialiser implementaton
        /// </summary>
        /// <param name="streamCompressor">IStreamCompression implementation composed via the IOC container</param>
        /// <param name="logger">ILogger implementation composed via the IOC container</param>    
        [InjectionConstructor]
        public JsonSerialiser(IStreamCompression streamCompressor, ILogger logger)
        {
            if (streamCompressor == null) throw new ArgumentNullException("streamCompressor");
            if (logger == null) throw new ArgumentNullException("logger");

            this._streamCompressor = streamCompressor;
            this._logger = logger;
        }

        /// <summary>
        /// Serialise the passed in object with the Json.Net serialiser
        /// </summary>
        /// <typeparam name="T">Generic type of the serialised object</typeparam>
        /// <param name="serialseObject">The object to be serialised</param>
        /// <returns>A serialised Json string</returns>
        public async Task<string> SerialiseAsync<T>(T serialseObject)
        {
            if (serialseObject == null) throw new ArgumentNullException("serialseObject");

            try
            {
                return await JsonConvert.SerializeObjectAsync(serialseObject);
            }
            catch (JsonSerializationException ex)
            {
                _logger.LogEntry(ex);
                throw new SerialisationException("Could Not Serialse The Object", ex);
            }
        }

        /// <summary>
        /// Serialise the passed in object with the Json.Net serialiser and compress the string using the IStreamCompression implementation
        /// </summary>
        /// <typeparam name="T">Generic type of the serialised object</typeparam>
        /// <param name="serialseObject">The object to be serialised</param>
        /// <returns>A compressed byte array of the serialised object</returns>
        public async Task<byte[]> SerailseAndCompressAsync<T>(T serialseObject)
        {
            if (serialseObject == null) throw new ArgumentNullException("serialseObject");

            try
            {
                string serialised = await SerialiseAsync(serialseObject);
                return await _streamCompressor.CompressStringAsync(serialised);
            }
            catch (StreamCompressionException ex)
            {
                _logger.LogEntry(ex);
                throw new SerialisationException("Could Not Compress The Object", ex);
            }
        }

        /// <summary>
        /// Deserialise the Json string into the generic object
        /// </summary>
        /// <typeparam name="T">Generic type of the serialised object</typeparam>
        /// <param name="serialseObject">The object to be serialised</param>
        /// <returns>A deserialsied object of type T</returns>
        public async Task<T> DeserialiseAsync<T>(string serialsedString)
        {
            if (serialsedString == null) throw new ArgumentNullException("serialsedString");

            try
            {
                return await JsonConvert.DeserializeObjectAsync<T>(serialsedString);
            }
            catch (JsonSerializationException ex)
            {
                _logger.LogEntry(ex);
                throw new SerialisationException("Could Not Deserialse The Object", ex);
            }
        }

        /// <summary>
        /// Uncompress and deserialise the Json string into the generic object
        /// </summary>
        /// <typeparam name="T">Generic type of the serialised object</typeparam>
        /// <param name="serialed">The object to be serialised</param>
        /// <returns>An uncompressed & deserialsied object of type T</returns>
        public async Task<T> DeserialseAndUnCompressAsync<T>(byte[] serialed)
        {
            if (serialed == null) throw new ArgumentNullException("serialed");

            try
            {
                string decompressedSerialised = await _streamCompressor.DecompressStringAsync(serialed);
                return await DeserialiseAsync<T>(decompressedSerialised);
            }
            catch (StreamCompressionException ex)
            {
                _logger.LogEntry(ex);
                throw new SerialisationException("Could Not Decompress The Object", ex);
            }
        }
    }
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ I don't see anything wrong with throwing the exception in the constructor. If the parameter is required, it's required. At least caught in the constructor you have a chance to review the call stack and find out why \$\endgroup\$
    – dreza
    Nov 10, 2012 at 3:39
  • 2
    \$\begingroup\$ I agree with dreza , constructing an object should be avoided if some of necessary parameter are not present during constructing an object. \$\endgroup\$
    – Paritosh
    Nov 12, 2012 at 7:07
  • 2
    \$\begingroup\$ As all methods in interface has async postfix, maybe rename ISerializer to IAsyncSerializer and remove async postfix in methods names. \$\endgroup\$
    – Danil
    Nov 14, 2012 at 7:25
  • \$\begingroup\$ surely it would be better to allow a null stringcompressor/logger rather than throw an exception in a constructor. It seems simple and logical that null wont log or compress? \$\endgroup\$
    – Ewan
    May 7, 2015 at 13:31

2 Answers 2

7
\$\begingroup\$

Code looks good, I like this IoC style. 3 points to your consideration:

  1. You should catch an AggregateException over await.
  2. I wouldn't bother passing a logger to a serializer - that's none of his business. Let the serializer throw if he's not happy.
  3. Fix some typo in names and messages ("Deserialse" and so).
  4. I somewhat doubt the whole concept of async serialization. I take serialized data to be an object snapshot in a known 'time point'. But if it's useful for you go for it.

(Oops didn't address your actual questions)

  1. Yes I think it's great.
  2. Sure. Lacking meaningful 'default object', you don't have many alternatives.
  3. This is probably the main issue here, and the hardest to answer. I have some doubts about returning a non-cancellable Task. I suspect if the object to be serialized has changed completely, the user may want to cancel the serialization.
\$\endgroup\$
0
\$\begingroup\$

A minor point, but what if you want to create a single threaded Serializer as well?

I would rename your interface to something like IAsyncSerializer so that you could have a different interface ISerializer.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.