2
\$\begingroup\$

I am building some classes to serialize (in an async / await way) potentially pretty long IEnumerable<T> over the network via the Stream class, I am wondering whether my code is easily readable et maintainable.

For example one purpose is that the serializer classes can be used in some ASP.NET controllers and write to the output stream a lot of data without wasting too many threads (hence the async / await approach).

In my example of implementation the item serialization leverages the ServiceStack.Text serializers.

using System;
using System.Collections.Generic;
using System.IO;
using System.Numerics;
using System.Text;
using System.Threading.Tasks;
using ServiceStack;

namespace ConsoleApp
{
    public static class Program
    {
        public static async Task Main(params string[] args)
        {
            var source = new[] {1, 2 ,3, 4};
            var serializer = new JsonLineBasedEnumerableStreamSerializer();

            using (var stream = new MemoryStream())
            {
                await serializer.SerializeToStreamAsync(source, stream, 1024, true);
                stream.Position = 0;
                using (var streamReader = new StreamReader(stream))
                {
                    var serialization = streamReader.ReadToEnd();
                    Console.WriteLine(serialization);
                }
            }
        }
    }

    public static class EnumerableExtensions
    {
        public static IEnumerable<Iteration<TSource>> Detailed<TSource>(this IEnumerable<TSource> source)
        {
            using (var enumerator = source.GetEnumerator())
            {
                var isFirst = true;
                var hasNext = enumerator.MoveNext();
                var index = BigInteger.Zero;
                while (hasNext)
                {
                    var current = enumerator.Current;
                    hasNext = enumerator.MoveNext();
                    yield return new Iteration<TSource>(index, current, isFirst, !hasNext);
                    isFirst = false;
                    index++;
                }
            }
        }
    }

    public readonly struct Iteration<T>
    {
        public readonly BigInteger Index;
        public readonly bool IsFirst;
        public readonly bool IsLast;
        public readonly T Value;

        public Iteration(BigInteger index, T value, bool isFirst, bool isLast)
        {
            Index = index;
            IsFirst = isFirst;
            IsLast = isLast;
            Value = value;
        }
    }

    public interface IEnumerableStreamSerializer
    {
        Task SerializeToStreamAsync<T>(IEnumerable<T> source, Stream stream, ushort bufferSize = 1024, bool leaveOpen = false);
    }

    public abstract class EnumerableStreamSerializer : IEnumerableStreamSerializer
    {
        public abstract Task SerializeToStreamAsync<T>(IEnumerable<T> source, Stream stream, ushort bufferSize = 1024, bool leaveOpen = false);
    }

    public abstract class LineBasedEnumerableStreamSerializer : EnumerableStreamSerializer
    {
        public async Task SerializeToStreamAsync<T>(IEnumerable<T> source, Stream stream, Encoding encoding, ushort bufferSize = 1024, bool leaveOpen = false)
        {
            using (var streamWriter = new StreamWriter(stream, encoding, bufferSize, leaveOpen))
            {
                var firstLine = await GetFirstLineAsync();
                await streamWriter.WriteLineAsync(firstLine);

                foreach (var iteration in source.Detailed())
                {
                    var itemLine = await GetItemLineAsync(iteration.Index, iteration.Value, iteration.IsFirst, iteration.IsLast);
                    await streamWriter.WriteLineAsync(itemLine);
                }

                var lastLine = await GetLastLineAsync();
                await streamWriter.WriteLineAsync(lastLine);
            }
        }

        public override async Task SerializeToStreamAsync<T>(IEnumerable<T> source, Stream stream, ushort bufferSize = 1024, bool leaveOpen = false)
        {
            await SerializeToStreamAsync(source, stream, Encoding.Default, bufferSize, leaveOpen);
        }

        protected abstract Task<string> GetFirstLineAsync();

        protected abstract Task<string> GetItemLineAsync<T>(BigInteger index, T item, bool isFirst, bool isLast);

        protected abstract Task<string> GetLastLineAsync();
    }

    public class JsonLineBasedEnumerableStreamSerializer : LineBasedEnumerableStreamSerializer
    {
        protected override async Task<string> GetFirstLineAsync() => 
            "[";

        protected override async Task<string> GetItemLineAsync<T>(BigInteger index, T item, bool isFirst, bool isLast)
        {
            var result = $"  {item.ToJson()}";

            return !isLast 
                ? $"{result}," 
                : result;
        }

        protected override async Task<string> GetLastLineAsync() => 
            "]";
    }

    public class XmlLineBasedEnumerableStreamSerializer : LineBasedEnumerableStreamSerializer
    {
        protected override async Task<string> GetFirstLineAsync() => 
            "<Array>";

        protected override async Task<string> GetItemLineAsync<T>(BigInteger index, T item, bool isFirst, bool isLast) => 
            $"  <Item>{item.ToXml()}</Item>";

        protected override async Task<string> GetLastLineAsync() => 
            "</Array>";
    }
}

What do you think, is there anything I can improve in my implementation to make it better?

\$\endgroup\$

1 Answer 1

7
\$\begingroup\$
  1. You have both IEnumerableStreamSerializer and EnumerableStreamSerializer, and in fact neither of them are used in the code you posted. I'd have just stuck with one or the other (probably the interface, and introduce the base class if you need to share common functionality; alternatively if you need to worry about backwards compatibility with other people implementing it, opt for the abstract base class).
  2. I'm not at all convinced that you need BigInteger. long goes up to nine quintillion - serializing something that large would mean transferring at least 36 exabytes of data (for json, more for xml). That's more than the entire world transfers over mobile data per month.
  3. Avoid Encoding.Default. It's based on the computer's code page, and will therefore be different across different computers. That's a very bad thing for a serialization library! A sensible, standard choice is Encoding.UTF8.
  4. You've got default parameter values for bufferSize and leaveOpen, but use an overload for encoding. That's inconsistent. Just use Encoding encoding = null, then check for null in your method.
  5. To be honest, I wouldn't make the buffer size configurable personally.
  6. There's no point in having an async method which just does await somethingElse; Just make it non-async and return somethingElse; instead.
  7. Your GetFirstLineAsync implementations will be throwing warnings, as they're async but don't have awaits. You'd be best off making them non-async, and returning a Task.FromResult("whatever"). You'll want to cache this Task, so you'll want a private static readonly Task<string> firstLine = Task.FromResult("whatever") then public Task<string> GetFirstLineAsync() => firstLine;
  8. Ditto GetLastLineAsync()
  9. Ditto GetItemLineAsync(), although in this case you can't cache the resulting Task.
  10. That said, do those methods really need to be async? Serializing something is very unlikely to be IO-bound - it's either going to be so cheap it's almost free, or CPU-bound. If it's CPU-bound, you're not going to call Task.Run for each line - that's crazy expensive and of questionable benefit. I'd ditch it - if it becomes expensive and you need to farm it off to another thread for whatever reason, run the entire operation on another thread, not individual lines. With that gone, your entire serializer becomes synchronous which goes against your question, but then your code (being synchronous in reality) goes against your question.
  11. If you're worried about speed, $"{result}," is going to be a bit slower than result + ",", and I'm not sure it's any more readable (of course, with more complex interpolated strings, you might want to take the performance hit for readability). Ditto the other bits of string interpolation.
  12. Missing null-checks on parameters etc, generally, if you care. If you add null-checks to EnumerableExtensions.Detailed, don't forget to move the state machine to a separate private (possibly inner) method.
\$\endgroup\$
2
  • \$\begingroup\$ thanks for your review! I started to think about this async all the way cause I am playing with the transformation of a remote resource content. Something like Remote File --> API (transforming each line of the remote file) --> Streaming of the transformation to the client. I kinda agree with you when it comes to my aggressive async policy but I still concern about the memory. The fact the remote file can be relatively big also means that the API may get stuck on one single thread while transforming the whole file at once. What do you think? \$\endgroup\$ Commented Apr 7, 2019 at 20:43
  • 1
    \$\begingroup\$ I forgot you were asynchronously writing to the stream when I wrote that bit if the review: that is worth doing asynchronously. If you need to asynchronously fetch each item to be serialised, fine, but then your source should probably yield items asynchronously (a la IAsyncEnumerable) and you should probably write out the previous serialised item while fetching the next. In that case as well, actually serializing each item, once fetched, will probably still be sync. That's a good deal more complex than the case your code shows, so you might want a separate serializer implementation. \$\endgroup\$
    – canton7
    Commented Apr 7, 2019 at 21:42

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.