2
\$\begingroup\$

I'm creating a download manager in C# (in combination with Unity 2019.1.8, running the .NET 4.x equivalent (MONO) scripting back end). To accomplish this I decided to start using interfaces and class inheritance through an (abstract) base class. As those are things I have never used before, To get to know how they work and learn to use them properly.

The below code works for my implementation and does what I expect it to do, but I am wondering if i'm using the interface and inheritance correctly (and if it is needed at all), or if I should adjust things to adhere to the rules around using these correctly. So my questions are mostly about the structure of the code, but other feedback is also appreciated!

IDownloadEntry: The interface I use to lay down the minimum requirements a DownloadEntry needs to have

using System.Collections.Generic;
using System.Net;

interface IDownloadEntry
{
    int CommandSequenceNumber { get; }
    string FileName { get; }

    HttpListenerResponse DownloadResponse { get; }

    List<byte> DownloadDataBuffer { get; set; }
    List<ResponseHeaderData> ResponseHeaders { get; set; }

    void AddToDataBuffer(byte[] data);
    void AddResponseHeader(ResponseHeaderData responseHeader);
    void ClearDataBuffer();

    string LogDownloadDataBuffer();
    string LogClassBuffer();
}

From this interface I create the abstract base class DownloadEntry which contains the implementations of the interface for the Properties and Methods I think every derived class needs to have implemented in order for it to function as a download type. DownloadEntry:

using System.Collections.Generic;
using System.Net;
using System.Text;

/// <summary>
/// Base class for the different DownloadEntry types
/// </summary>
public abstract class DownloadEntry : IDownloadEntry
{
    //CommandSequenceNumber is used to track a specific download
    public int CommandSequenceNumber { get; private set; }
    public string FileName { get; private set; }
    public HttpListenerResponse DownloadResponse { get; private set; }

    //The data downloaded
    private List<byte> downloadDataBuffer;
    public List<byte> DownloadDataBuffer
    {
        get
        {
            if (downloadDataBuffer == null) downloadDataBuffer = new List<byte>();
            return downloadDataBuffer;
        }
        set
        {
            downloadDataBuffer = value;
        }
    }

    //The responses send by the server containing information about the send packets
    private List<ResponseHeaderData> responseHeaders;
    public List<ResponseHeaderData> ResponseHeaders
    {
        get
        {
            if (responseHeaders == null) responseHeaders = new List<ResponseHeaderData>();
            return responseHeaders;
        }
        set
        {
            responseHeaders = value;
        }
    }

    /// <summary>
    /// Create a new base DownloadEntry
    /// </summary>
    /// <param name="CommandSequenceNumber">The csn associated with this download</param>
    /// <param name="FileName">The file name of the file downloaded</param>
    /// <param name="DownloadResponse">The response over which the data can be send</param>
    public DownloadEntry(int CommandSequenceNumber, string FileName, HttpListenerResponse DownloadResponse)
    {
        this.CommandSequenceNumber = CommandSequenceNumber;
        this.FileName = FileName;
        this.DownloadResponse = DownloadResponse;

        UnityEngine.Debug.LogFormat("<color=#00aa00>Base Constructor:</color> Created type: {0}: {1}", GetType().Name, ToString());
    }

    public void AddResponseHeader(ResponseHeaderData responseHeader) => responseHeaders.Add(responseHeader);
    public void ClearResponseHeader() => responseHeaders.Clear();

    public void AddToDataBuffer(byte[] data) => downloadDataBuffer.AddRange(data);
    public void ClearDataBuffer() => downloadDataBuffer.Clear();

    /// <summary>
    /// Get the ASCII representation of the data found in the data buffer
    /// </summary>
    /// <returns>ASCII string of the DownloadDataBuffer</returns>
    public string LogDownloadDataBuffer() => Encoding.ASCII.GetString(DownloadDataBuffer.ToArray());

    public abstract string LogClassBuffer();

    public override string ToString() => string.Format("CommandSequenceNumber: {0}, FileName: {1}, Data.length: {2}", CommandSequenceNumber, FileName, DownloadDataBuffer.Count);
}

I inherit from this base class when creating the download classes themselves, which I have divided into four categories:

  • RunningDownloadEntry for downloads that are active.
  • CompletedDownloadEntry for downloads that have successfully completed.
  • FailedDownloadEntry for downloads that encoutered an error and could not finish
  • CanceledDownloadEntry for downloads that have been canceled by the user.

being implemented like this:

using System.Collections.Generic;
using System.Net;
using System.Text;


/// <summary>
/// Class used for downloads that have been completed without Error or cancelation
/// </summary>
public sealed class CompletedDownloadEntry : DownloadEntry
{
    internal CompletedDownloadEntry(int CommandSequenceNumber, string FileName, HttpListenerResponse DownloadResponse)
        : base(CommandSequenceNumber, FileName, DownloadResponse) { }

    internal byte[] CompletionBuffer { get; private set; }
    internal void SetCompletionBuffer(byte[] data) => CompletionBuffer = data;
    internal void ClearCompletionBuffer() => CompletionBuffer = new byte[0];
    public override string LogClassBuffer() => string.Format("CompletionBuffer: {0}", Encoding.ASCII.GetString(CompletionBuffer));
}

/// <summary>
/// Class used to store downloads that have failed with an error, and won't have any usable data.
/// </summary>
public sealed class FailedDownloadEntry : DownloadEntry
{
    internal FailedDownloadEntry(int CommandSequenceNumber, string FileName, HttpListenerResponse DownloadResponse)
        : base(CommandSequenceNumber, FileName, DownloadResponse)
    {
        UnityEngine.Debug.LogErrorFormat("Download {0} failed: {1}", ToString(), LogClassBuffer());
    }

    internal byte[] ErrorBuffer { get; private set; }
    internal void SetErrorBuffer(byte[] data) => ErrorBuffer = data;
    internal void ClearErrorBuffer() => ErrorBuffer = new byte[0];
    public override string LogClassBuffer() => string.Format("ErrorBuffer: {0}", Encoding.ASCII.GetString(ErrorBuffer));
}

/// <summary>
/// Class used for downloads that have been canceled but still contain usuable data
/// </summary>
public sealed class CanceledDownloadEntry : DownloadEntry
{
    internal CanceledDownloadEntry(int CommandSequenceNumber, string FileName, HttpListenerResponse DownloadResponse)
        : base(CommandSequenceNumber, FileName, DownloadResponse) { }

    internal byte[] CanceledBuffer { get; private set; }
    internal void SetCanceledBuffer(byte[] data) => CanceledBuffer = data;
    internal void ClearCanceledBuffer() => CanceledBuffer = new byte[0];
    public override string LogClassBuffer() => string.Format("CanceledBuffer: {0}", Encoding.ASCII.GetString(CanceledBuffer));
}

This is the implementation:

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Threading;
using UnityEngine;

public class ServerConnect : MonoBehaviour
{
    internal static bool applicationRunning = true;

    internal static TcpClient client;
    internal static HttpListener listener;
    internal static HttpListenerContext listenerContext;

    private static string uri = "0.0.0.0";
    internal static DownloadManager dlManager;

    internal static string LocalIP { get; private set; }
    static string GetLocalIPAddress()
    {
        IPHostEntry host = Dns.GetHostEntry(Dns.GetHostName());
        foreach (IPAddress ip in host.AddressList)
        {
            if (ip.AddressFamily == AddressFamily.InterNetwork)
            {
                return ip.ToString();
            }
        }
        throw new Exception("No available IPv4 network was found");
    }

    internal static int LocalPort { get; private set; }
    private static int GetLocalOpenPort()
    {
        TcpListener l = new TcpListener(IPAddress.Loopback, 0);
        l.Start();
        int port = ((IPEndPoint)l.LocalEndpoint).Port;
        l.Stop();
        return port;
    }

    private void Start()
    {
        dlManager = new DownloadManager();

        LocalIP = GetLocalIPAddress();
        LocalPort = GetLocalOpenPort();

        Thread openConnectionThread = new Thread(() => OpenConnection());
        openConnectionThread.Start();

        Thread setupListenerThread = new Thread(() => SetupHttpListener());
        setupListenerThread.Start();
    }

    private void OnApplicationQuit()
    {
        client.Close();
        applicationRunning = false;
    }

    private static void OpenConnection()
    {
        IPAddress hostAddress = Dns.GetHostAddresses(uri)[0];
        IPEndPoint hostEndPoint = new IPEndPoint(hostAddress, 8080);

        client = new TcpClient(AddressFamily.InterNetwork);
    }

    static void SetupHttpListener()
    {
        listener = new HttpListener();
        listener.Prefixes.Add(string.Format("http://{0}:{1}/", LocalIP, LocalPort));
        listener.Start();

        Thread listenerThread = new Thread(ListenOnHttp);
        listenerThread.Start();
    }

    static void ListenOnHttp()
    {
        Debug.Log("Starting HttpListener on threadID: " + Thread.CurrentThread.ManagedThreadId);
        //We want the listener to be listening at all times
        while (applicationRunning)
        {
            if (!listener.IsListening)
            {
                Debug.LogErrorFormat("Listener has stopped listening");
            }
            else
            {
                IAsyncResult result = listener.BeginGetContext(new AsyncCallback(HttpListenerCallback), listener);
                //Wait until a new request has been send over http...
                result.AsyncWaitHandle.WaitOne(-1, false);
            }
        }
    }

    private static void HttpListenerCallback(IAsyncResult ar)
    {
        HttpListener listener = (HttpListener)ar.AsyncState;
        var downloadContext = listener.EndGetContext(ar);
        string requestedFileName = downloadContext.Request.Url.AbsolutePath.Substring(1);

        HttpListenerResponse response = downloadContext.Response;

        dlManager.CreateVideoFileRequest(requestedFileName, response);
    }
}

public class DownloadManager
{    
    internal List<RunningDownloadEntry> runningDownloads = new List<RunningDownloadEntry>();
    internal List<CompletedDownloadEntry> completedDownloads = new List<CompletedDownloadEntry>();
    internal List<FailedDownloadEntry> failedDownloads = new List<FailedDownloadEntry>();
    internal List<CanceledDownloadEntry> canceledDownloads = new List<CanceledDownloadEntry>();

    private static int commandSequenceNumber = 10000;
    internal int GetNewCommandSequenceNumber() => ++commandSequenceNumber;

    internal DownloadManager()
    {
        //Start a thread on which the responses from the server can be downloaded
        Thread downloadResponseThread = new Thread(DownloadResponseListener);
        downloadResponseThread.Start();
    }

    private void DownloadResponseListener()
    {
        while (ServerConnect.applicationRunning)
        {
            if (ServerConnect.client?.Available > 0)
            {
                ReadDownloadResponse();
            }
        }
    }

    private void SendServerRequest(byte[] data)
    {
        //Send the request to the server
        NetworkStream stream = ServerConnect.client.GetStream();
        stream.Write(data, 0, data.Length);
    }

    internal void CreateVideoFileRequest(string fileName, HttpListenerResponse response)
    {
        //Create the request string
        string requestString = string.Format("GET_FILE {0}", fileName);
        byte[] message = Encoding.ASCII.GetBytes(requestString);
        int requestCommandSequenceNumber = GetNewCommandSequenceNumber();

        //Wrap the request in a header that the server can validate
        GeneralHeaderData requestWithHeader = new GeneralHeaderData(message.Length, GeneralHeaderData.PacketType.Download_Control, requestCommandSequenceNumber, message);
        byte[] serializedRequest = requestWithHeader.SerializeHeader();

        //Create a new download entry for the requested file
        RunningDownloadEntry downloadEntry = new RunningDownloadEntry(requestCommandSequenceNumber, fileName, response);
        runningDownloads.Add(downloadEntry);

        SendServerRequest(serializedRequest);
    }

    private void ReadDownloadResponse()
    {
        //Create an empty response header
        var packetSize = -1;
        GeneralHeaderData.PacketType module = GeneralHeaderData.PacketType.Undefined;
        var sequenceNumber = -1;

        //Get the header from the first 12 bytes
        GetNewDataPacketHeader(ref packetSize, ref module, ref sequenceNumber);

        //Wait for the client to have enough data available to downlaod the entire package in one go. This *shouldn't* be necessary, but it is...
        while (ServerConnect.client.Available < packetSize - 12)
        {
            Thread.Sleep(0);
        }

        //Get the matching DownloadEntry from the runningDownloads based on the commandSequenceNumber found in the header
        RunningDownloadEntry currentDownload = null;

        try
        {
            currentDownload = runningDownloads.Where(o => o.CommandSequenceNumber == sequenceNumber).FirstOrDefault();
        }
        catch (Exception e)
        {
            //throw new NullReferenceException();
            Debug.LogErrorFormat("e:{0}", e);
            return;
        }

        //Determine how much data needs to be read
        var bytesLeftToRead = packetSize - 12;
        var totalBytesRead = 0;
        var bytesReadThisPacket = 0;
        var dataReadBuffer = new byte[bytesLeftToRead];

        //Read all the data off the NetworkStream
        NetworkStream stream = ServerConnect.client.GetStream();
        while (totalBytesRead < bytesLeftToRead)
        {
            bytesReadThisPacket = stream.Read(dataReadBuffer, totalBytesRead, bytesLeftToRead);

            totalBytesRead += bytesReadThisPacket;
            bytesLeftToRead -= bytesReadThisPacket;
        }

        ReadResponseMessage(dataReadBuffer, module, currentDownload);
    }

    static void GetNewDataPacketHeader(ref int packetSize, ref GeneralHeaderData.PacketType module, ref int sequenceNumber)
    {
        byte[] headerBuffer = new byte[12];

        int numberOfHeaderBytesRead = 0;

        //Wait for the server to atleast have 12 bytes available so it can download an entire header
        while (ServerConnect.client.Available < 12)
        {
            Thread.Sleep(0);
        }

        //Download the header
        while (numberOfHeaderBytesRead < 12)
        {
            NetworkStream stream = ServerConnect.client.GetStream();
            numberOfHeaderBytesRead = stream.Read(headerBuffer, numberOfHeaderBytesRead, headerBuffer.Length - numberOfHeaderBytesRead);
        }

        //Extract the data from the header
        packetSize = BitConverter.ToInt32(headerBuffer, 0);
        module = (GeneralHeaderData.PacketType)BitConverter.ToInt32(headerBuffer, 4);
        sequenceNumber = BitConverter.ToInt32(headerBuffer, 8);
    }

    private void ReadResponseMessage(byte[] data, GeneralHeaderData.PacketType module, RunningDownloadEntry currentDownload)
    {
        switch (module)
        {
            //Packet types marked as Download_Control only contain data about the oncoming, or peviousy downloaded file data, but no actual file data.
            case GeneralHeaderData.PacketType.Download_Control:

                var responseMessage = Encoding.ASCII.GetString(data);
                //If we encounter an error we remove the current download from the running list and add it to the error list. Closes the outputstream
                if (responseMessage.Contains("error"))
                {
                    Debug.LogErrorFormat("Moving {0} to failedDownloads from ERROR case", currentDownload.ToString());
                    currentDownload.SetErrorBuffer(data);
                    Debug.LogErrorFormat("Error: {0}", currentDownload.LogErrorBuffer());
                    currentDownload.DownloadResponse.OutputStream.Close();

                    failedDownloads.Add((FailedDownloadEntry)currentDownload);
                    runningDownloads.Remove(currentDownload);
                }
                //DELETED signifies that the entire file has been transfered, moves the currentDownload to the completed list
                //Writes all the data to the response.outputstream and closes the stream.
                else if (responseMessage.Contains("deleted"))
                {
                    currentDownload.SetCompletionBuffer(data);

                    string s = Encoding.ASCII.GetString(currentDownload.AcknowledgeBuffer.ToArray());
                    if (s.Contains("cancel"))
                    {
                        currentDownload.DownloadResponse.OutputStream.Close();
                        canceledDownloads.Add((CanceledDownloadEntry)currentDownload);
                        runningDownloads.Remove(currentDownload);
                        return;
                    }
                    currentDownload.SetCompletionBuffer(data);

                    try
                    {
                        if (runningDownloads.Contains(currentDownload))
                        {
                            currentDownload.DownloadResponse.OutputStream.Write(currentDownload.DownloadDataBuffer.ToArray(), 0, currentDownload.DownloadDataBuffer.Count);
                        }
                        else
                        {
                            Debug.LogErrorFormat("CurrentDownload was not in runningDownloads, not writing data");
                        }
                    }
                    catch (IOException e)
                    {
                        Debug.LogErrorFormat("OutputStream.Write:: {0}", e);
                        Debug.LogErrorFormat("current download: {0}", currentDownload.ToString());
                        currentDownload.DownloadResponse.OutputStream.Close();
                    }

                    currentDownload.DownloadResponse.OutputStream.Close();

                    completedDownloads.Add((CompletedDownloadEntry)currentDownload);
                    runningDownloads.Remove(currentDownload);
                }
                else if (responseMessage.Contains("ok"))
                {
                    currentDownload.AddToAcknowledgeBuffer(data);

                }
                else if (responseMessage.Contains(("prop")))
                {
                    currentDownload.SetPropertiesBuffer(data);
                }
                break;

            //Packets marked Download_Data only contain a 12 byte header and data. Add the downloaded data to the currentDownload's DataBuffer
            case GeneralHeaderData.PacketType.Download_Data:

                //Extract the 12 byte header containing the package number and max package number
                try
                {
                    if (runningDownloads.Contains(currentDownload))
                    {
                        currentDownload.AddToDataBuffer(data.Skip(12).ToArray());
                    }
                }
                catch (Exception e)
                {
                    Debug.LogErrorFormat("DownloadData:: {0}", e);
                }
                break;

            default:
                break;
        }
    }
}

public class GeneralHeaderData
{
    private readonly int packetSize;
    public enum PacketType { Undefined = 0, General = 1, Config = 2, Database = 3, Download_Control = 4, Download_Data = 5, Upload_Control = 6, Upload_Data = 7 };
    private readonly PacketType packetType;
    private readonly int packetCommandSequenceNumber;
    private readonly List<byte> packetData = new List<byte>();

    public GeneralHeaderData(int size, PacketType type, int sequenceNumber, byte[] data)
    {
        //we add +12 to the size because that is the total amount of bytes used for the header itself (3x int32 which is 4bytes * 3)
        packetSize = size + 12;
        packetType = type;
        packetCommandSequenceNumber = sequenceNumber;
        packetData = data.ToList();
    }

    public byte[] SerializeHeader()
    {
        List<byte> packetAsBytes = new List<byte>();
        packetAsBytes.AddRange(BitConverter.GetBytes(packetSize));
        packetAsBytes.AddRange(BitConverter.GetBytes((int)packetType));
        packetAsBytes.AddRange(BitConverter.GetBytes(packetCommandSequenceNumber));
        packetAsBytes.AddRange(packetData);

        byte[] result = packetAsBytes.ToArray();
        return result;
    }
}

[System.Serializable]
public class ResponseHeaderData
{
    public enum Mp4DataPart { WholeFile = 0, Ftyp_Moov = 1, Ftyp = 2, Moov = 3, Mfra = 4, AllFragments = 5, Fragment = 6 };
    public Mp4DataPart mp4DataPart { get; private set; }
    public int BlockSequenceNumber { get; private set; }
    public int TotalNumberOfBlocks { get; private set; }

    public ResponseHeaderData(byte[] responseHeader)
    {
        mp4DataPart = (Mp4DataPart)BitConverter.ToInt32(responseHeader, 0);
        BlockSequenceNumber = BitConverter.ToInt32(responseHeader, 4);
        TotalNumberOfBlocks = BitConverter.ToInt32(responseHeader, 8);
    }
}

I have not further implemented the handling of Entries after they have been sorted, but that would be out of scope for this question.

My question(s):

  • Am I using interface correctly/does it make sense to use in this case?
  • Am I using inheritance from an abstract base class correctly? I went with abstract to force inheritors to implement their own LogClassBuffer method.
  • To be able to cast a RunningDownloadEntry to any of the other three I implemented my own User-defined conversion operators. Is this a proper way to do this?
\$\endgroup\$
  • \$\begingroup\$ Could you show us how you are using these classes and how you switch e.g. from RunningDownloadEntry to let's say FailedDownloadEntry? \$\endgroup\$ – t3chb0t Oct 3 at 16:49
  • \$\begingroup\$ Can you post the real code? This one is heavily edited; many parts are missing and the ones that are there use invalid syntax and contain many typos. \$\endgroup\$ – t3chb0t Oct 4 at 6:55
  • 1
    \$\begingroup\$ @t3chb0t I've added the actual implementation, I had to remove any sensitive data like ip addresses and validation though. I hope this is enough information to work with, as i'm not able to provide any further information. \$\endgroup\$ – remy_rm Oct 4 at 8:07
  • 1
    \$\begingroup\$ I'd say this is enough and it looks good now ;-] \$\endgroup\$ – t3chb0t Oct 4 at 8:57
2
\$\begingroup\$
  1. There's clearly no value in the interface.
  2. Although I'm a proponent of using inheritance only in cases when you need to exercise dynamic polymorphism and this is not a case, your abstract class looks kinda ok as implementations are pretty similar and there's a lot of them. Still, you might consider using composition which is preferrable. Making LogClassBuffer abstract is a neat trick and widely used for abstract classes. The only real downfall I see is that your classes do not conform Liskov substitution principle. I would suggest you to rename your methods to SetBuffer, ClearBuffer and move them up to abstract class.
  3. I guess there's not much wrong with casts. I'm not sure whether smth like this could work too for your case
internal CanceledDownloadEntry(DownloadEntry entry) 
{
    //your cast
}

You might want to investigate it at some point 4. This not relates to a question but you can replace this

currentDownload = runningDownloads.Where(o => o.CommandSequenceNumber == sequenceNumber).FirstOrDefault();

with this

currentDownload = runningDownloads.FirstOrDefault(o => o.CommandSequenceNumber == sequenceNumber);
\$\endgroup\$
  • \$\begingroup\$ I will definitely look through these resources you've provided. A question about There's clearly no value in the interface.. Would it have made sense to use the interface if I hadn't also used the base class? So that i'd make my classes using RunningDownloadEntry : IDownloadEntry FailedDownloadEntry : IDownloadEntry etc? Meaning using both a base class and interface makes the interface redundant? \$\endgroup\$ – remy_rm Oct 4 at 14:07
  • \$\begingroup\$ Both class and interface are definitely redundant. Surely, this was my main motivation. But that's not the only my point. Interfaces are generally useful when you employ dynamic dispatch mentioned in my above answer or in order to facilitate unit testing. Both are not the case for the code you've presented. \$\endgroup\$ – Bohdan Stupak Oct 4 at 14:23
  • \$\begingroup\$ Thank you for the information and answer! \$\endgroup\$ – remy_rm Oct 4 at 14:25
  • \$\begingroup\$ You're welcome! \$\endgroup\$ – Bohdan Stupak Oct 4 at 15:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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