Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm just starting to develop more in C# after being mainly a VB.Net developer and was looking for some one to critic 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);
            }
        }
    }
}
share|improve this question
2  
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 –  dreza Nov 10 '12 at 3:39
2  
I agree with dreza , constructing an object should be avoided if some of necessary parameter are not present during constructing an object. –  paritosh Nov 12 '12 at 7:07
2  
As all methods in interface has async postfix, maybe rename ISerializer to IAsyncSerializer and remove async postfix in methods names. –  Danil Nov 14 '12 at 7:25
add comment

1 Answer 1

up vote 4 down vote accepted

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.
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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