12
\$\begingroup\$

I've been writing a small library that allows for easy querying of the Stack Exchange websockets.

I'm going to add an enum to replace the manual SiteId at every RequestParameter so you can let that one slide (unless you have something else in mind).

A few thoughts I have:

  • RequestParameters defines its ResponseDataType as JsonConverter while in reality I would prefer it to be DataConverter<?> (which Java has). Can I go more specific? (I don't want the caller to have to define it)

  • Even though it's internal, I feel like having a DataConverter<T> field in my RequestParameters is questionable. I am delegating the way I handle the request to the request itself. Granted, it can't be modified in any way but are there better options?

Program.cs

namespace Console
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            //ActiveQuestions();
            NewestQuestionsByTag();

            System.Console.ReadKey();
        }

        #region ActiveQuestions

        private static void ActiveQuestions()
        {
            var settings = new ActiveQuestionsRequestParameters
            {
                SiteId = "155"
            };
            var socket = new StackSocket("wss://qa.sockets.stackexchange.com", settings);
            socket.OnSocketReceive += OnActiveQuestionsDataReceived;
            socket.Connect();
        }

        private static void OnActiveQuestionsDataReceived(object sender, SocketEventArgs e)
        {
            var data = e.Response.Data as ActiveQuestionsData;
            if (data == null)
            {
                return;
            }

            System.Console.WriteLine("{0} - {1}", "Title", data.TitleEncodedFancy);
            System.Console.WriteLine("{0} - {1}", "Tags", string.Join(", ", data.Tags));
            System.Console.WriteLine("{0} - {1}", "Last activity", data.LastActivityDate);
            System.Console.WriteLine("{0}", data.ApiSiteParameter);
            System.Console.WriteLine(data.QuestionUrl);
            System.Console.WriteLine();
        }

        #endregion

        #region NewestQuestionsByTag

        private static void NewestQuestionsByTag()
        {
            var settings = new NewestQuestionsByTagRequestParameters
            {
                SiteId = "1",
                Tag = "Java"
            };
            var socket = new StackSocket("wss://qa.sockets.stackexchange.com", settings);
            socket.OnSocketReceive += OnNewestQuestionsByTagDataReceived;
            socket.Connect();
        }

        private static void OnNewestQuestionsByTagDataReceived(object sender, SocketEventArgs e)
        {
            var data = e.Response.Data as NewestQuestionsByTagData;
            if (data == null)
            {
                return;
            }

            System.Console.WriteLine("{0} - {1}", "Title", data.Body);
            System.Console.WriteLine("{0} - {1}", "Tags", string.Join(", ", data.Tags));
            System.Console.WriteLine("{0} - {1}", "ID", data.Id);
            System.Console.WriteLine("{0} - {1}", "Site ID", data.SiteId);
            System.Console.WriteLine(data.Fetch);
            System.Console.WriteLine();
        }

        #endregion
    }
}

RequestParameters.cs

namespace Library.Requests
{
    public abstract class RequestParameters
    {
        internal abstract string GetRequestValue();

        internal abstract JsonConverter ResponseDataType { get; }
    }

    public sealed class ActiveQuestionsRequestParameters : RequestParameters
    {
        public string SiteId { get; set; }

        internal override string GetRequestValue()
        {
            return SiteId + "-questions-active";
        }

        internal override JsonConverter ResponseDataType
        {
            get { return new DataConverter<ActiveQuestionsData>(); }
        }
    }

    public sealed class NewestQuestionsByTagRequestParameters : RequestParameters
    {
        public string SiteId { get; set; }
        public string Tag { get; set; }

        internal override string GetRequestValue()
        {
            return SiteId + "-questions-newest-tag-" + Tag.ToLower();
        }

        internal override JsonConverter ResponseDataType
        {
            get { return new DataConverter<NewestQuestionsByTagData>(); }
        }
    }
}

Response.cs

namespace Library.Responses
{
    public sealed class Response
    {
        [JsonProperty("action")]
        public string Action { get; internal set; }

        [JsonProperty("data")]
        public Data Data { get; internal set; }
    }

    public abstract class Data
    {
    }

    public sealed class ActiveQuestionsData : Data
    {
        [JsonProperty("siteBaseHostAddress")]
        public string SiteBaseHostAddress { get; internal set; }

        [JsonProperty("id")]
        public string Id { get; internal set; }

        [JsonProperty("titleEncodedFancy")]
        public string TitleEncodedFancy { get; internal set; }

        [JsonProperty("bodySummary")]
        public string BodySummary { get; internal set; }

        [JsonProperty("tags")]
        public IEnumerable<string> Tags { get; internal set; }

        [JsonProperty("lastActivityDate")]
        [JsonConverter(typeof (EpochTimeConverter))]
        public DateTime LastActivityDate { get; internal set; }

        [JsonProperty("url")]
        [JsonConverter(typeof (UriConverter))]
        public Uri QuestionUrl { get; internal set; }

        [JsonProperty("ownerUrl")]
        [JsonConverter(typeof (UriConverter))]
        public Uri OwnerUrl { get; internal set; }

        [JsonProperty("ownerDisplayName")]
        public string OwnerDisplayName { get; internal set; }

        [JsonProperty("apiSiteParameter")]
        public string ApiSiteParameter { get; internal set; }
    }

    public sealed class NewestQuestionsByTagData : Data
    {
        [JsonProperty("id")]
        public string Id { get; internal set; }

        [JsonProperty("body")]
        public string Body { get; internal set; }

        [JsonProperty("tags")]
        public IEnumerable<string> Tags { get; internal set; }

        [JsonProperty("siteid")]
        public string SiteId { get; internal set; }

        [JsonProperty("fetch")]
        public bool Fetch { get; internal set; }
    }
}

DataConverter.cs

namespace Library.Utilities
{
    internal sealed class DataConverter<T> : JsonConverter
    {
        public override bool CanConvert(Type objectType)
        {
            return objectType == typeof (Data);
        }

        public override object ReadJson(JsonReader reader, Type objectType, object existingValue,
            JsonSerializer serializer)
        {
            var value = reader.Value as string;
            return JsonConvert.DeserializeObject<T>(value);
        }

        public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
        {
            throw new NotImplementedException();
        }
    }
}

EpochTimeConverter.cs

namespace Library.Utilities
{
    internal sealed class EpochTimeConverter : JsonConverter
    {
        public override bool CanConvert(Type objectType)
        {
            return objectType == typeof (DateTime);
        }

        public override object ReadJson(JsonReader reader, Type objectType, object existingValue,
            JsonSerializer serializer)
        {
            long timestamp;
            long.TryParse(reader.Value.ToString(), out timestamp);
            return new DateTime(1970, 1, 1).AddSeconds(timestamp);
        }

        public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
        {
            throw new NotImplementedException();
        }
    }
}

UriConverter

namespace Library.Utilities
{
    internal sealed class UriConverter : JsonConverter
    {
        public override bool CanConvert(Type objectType)
        {
            return objectType == typeof (Uri);
        }

        public override object ReadJson(JsonReader reader, Type objectType, object existingValue,
            JsonSerializer serializer)
        {
            return new Uri(reader.Value.ToString());
        }

        public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
        {
            throw new NotImplementedException();
        }
    }
}

StackSocket.cs

namespace Library
{
    public sealed class StackSocket : IDisposable
    {
        private readonly ClientWebSocket _socket = new ClientWebSocket();
        private readonly Uri _uri;
        private readonly RequestParameters _requestParameters;
        private const int BufferSize = 4096;
        private const int BufferAmplifier = 20;

        public event EventHandler<SocketEventArgs> OnSocketReceive;

        public StackSocket(string url, RequestParameters parameters)
        {
            _uri = new Uri(url);
            _requestParameters = parameters;
        }

        public async void Connect()
        {
            if (_socket.State != WebSocketState.Open && _socket.State != WebSocketState.Connecting)
            {
                await _socket.ConnectAsync(_uri, CancellationToken.None);
            }

            var request = Encoding.UTF8.GetBytes(_requestParameters.GetRequestValue());

            await
                _socket.SendAsync(new ArraySegment<byte>(request), WebSocketMessageType.Text, true,
                    CancellationToken.None);

            await Receive();
        }

        private async Task Receive()
        {
            var temporaryBuffer = new byte[BufferSize];
            var buffer = new byte[BufferSize*BufferAmplifier];
            var offset = 0;

            while (_socket.State == WebSocketState.Open)
            {
                WebSocketReceiveResult response;

                while (true)
                {
                    response =
                        await _socket.ReceiveAsync(new ArraySegment<byte>(temporaryBuffer), CancellationToken.None);
                    temporaryBuffer.CopyTo(buffer, offset);
                    offset += response.Count;
                    temporaryBuffer = new byte[BufferSize];

                    if (response.EndOfMessage)
                    {
                        break;
                    }
                }

                if (response.MessageType == WebSocketMessageType.Close)
                {
                    await
                        _socket.CloseAsync(WebSocketCloseStatus.NormalClosure, "Close response received",
                            CancellationToken.None);
                }
                else
                {
                    var result = Encoding.UTF8.GetString(buffer);
                    var responseObject = JsonConvert.DeserializeObject<Response>(result,
                        _requestParameters.ResponseDataType);

                    OnSocketReceive.Invoke(this, new SocketEventArgs {Response = responseObject});
                    buffer = new byte[BufferSize*BufferAmplifier];
                    offset = 0;
                }
            }
        }

        public void Dispose()
        {
            _socket.Dispose();
        }
    }

    public class SocketEventArgs : EventArgs
    {
        public Response Response { get; set; }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Are you not using System;? Why is System.Console fully qualified? \$\endgroup\$ – Mathieu Guindon May 29 '14 at 13:38
  • \$\begingroup\$ The namespace it's in is Console. A lack of imagination when it comes to naming my console project. \$\endgroup\$ – Jeroen Vannevel May 29 '14 at 14:06
  • \$\begingroup\$ Right, missed that namespace Console ;) \$\endgroup\$ – Mathieu Guindon May 29 '14 at 14:18
6
+50
\$\begingroup\$

I will look at this again in a couple of days or maybe even later today, but here is something I saw right at the end of the question

            WebSocketReceiveResult response;

            while (true)
            {
                response =
                    await _socket.ReceiveAsync(new ArraySegment<byte>(temporaryBuffer), CancellationToken.None);
                temporaryBuffer.CopyTo(buffer, offset);
                offset += response.Count;
                temporaryBuffer = new byte[BufferSize];

                if (response.EndOfMessage)
                {
                    break;
                }
            }

you can get rid of that pesky if statement if you write the while loop as a do while and use the condition response.EndOfMessage

WebSocketReceiveResult response;

Do
{
    response =
        await _socket.ReceiveAsync(new ArraySegment<byte>(temporaryBuffer), CancellationToken.None);
        temporaryBuffer.CopyTo(buffer, offset);
        offset += response.Count;
        temporaryBuffer = new byte[BufferSize];
} While(!response.EndOfMessage)

I don't like this

if (data == null)
{
    return;
}

it's just looks blah.

you should at least let this print to the Console that the data was empty, both places that you used this if the data isn't null you print out information to the console, why wouldn't you want to know that the data was null? Print it out that the data was null.


I was just looking at this

    private static void OnActiveQuestionsDataReceived(object sender, SocketEventArgs e)
    {
        var data = e.Response.Data as ActiveQuestionsData;
        if (data == null)
        {
            return;
        }

        System.Console.WriteLine("{0} - {1}", "Title", data.TitleEncodedFancy);
        System.Console.WriteLine("{0} - {1}", "Tags", string.Join(", ", data.Tags));
        System.Console.WriteLine("{0} - {1}", "Last activity", data.LastActivityDate);
        System.Console.WriteLine("{0}", data.ApiSiteParameter);
        System.Console.WriteLine(data.QuestionUrl);
        System.Console.WriteLine();
    }

and I think that if you aren't going to write anything to the console then you should restructure this so that it looks like this instead

    private static void OnActiveQuestionsDataReceived(object sender, SocketEventArgs e)
    {
        var data = e.Response.Data as ActiveQuestionsData;
        if (data != null)
        {
            System.Console.WriteLine("{0} - {1}", "Title", data.TitleEncodedFancy);
            System.Console.WriteLine("{0} - {1}", "Tags", string.Join(", ", data.Tags));
            System.Console.WriteLine("{0} - {1}", "Last activity", data.LastActivityDate);
            System.Console.WriteLine("{0}", data.ApiSiteParameter);
            System.Console.WriteLine(data.QuestionUrl);
            System.Console.WriteLine();
         }
    }

this is straight forward what is going on.

I know a lot of people don't like using negative conditionals in their if statement, but if you aren't doing anything on Null other than exiting the method this is the cleanest way to do that.

\$\endgroup\$
  • 3
    \$\begingroup\$ Cheers, I adapted both of those things. The loop does look a lot cleaner now. \$\endgroup\$ – Jeroen Vannevel May 25 '14 at 16:59
  • \$\begingroup\$ glad it helped out, even if it's only a small thing \$\endgroup\$ – Malachi May 26 '14 at 4:49
  • 1
    \$\begingroup\$ I disagree about the last thing. It quickly leads to extreme nesting. Returning on null values early makes it clear early on that the method exits and prevents indenting code. It's a matter of taste, of course, but this is how I feel and my arguments for it (I don't care for the negative condition, by the way). \$\endgroup\$ – Ingo Bürk May 29 '14 at 14:45
  • 1
    \$\begingroup\$ I have to agree with @IngoBürk here, the code is clearer with the data == null positive conditional and less nesting - the non-nested code immediately strikes one as what the method is supposed to be doing, the "happy path". In the linked comment the OP's code was going to have that nesting level no matter how the condition was phrased; it's not the case here. \$\endgroup\$ – Mathieu Guindon May 29 '14 at 15:03
  • 3
    \$\begingroup\$ It should be noted that that's just some testing code to get output; it's not really a part of the program in the first place but more intended to show how it would be used. I prefer guard clauses myself as well so I'm sticking to == null but I've changed it into an exception for clarity. \$\endgroup\$ – Jeroen Vannevel May 29 '14 at 16:49
3
\$\begingroup\$

Minor annoyance:

namespace Console

If you're using System;, you've just asked for name clashes with your top-level namespace named like this. Hence:

        System.Console.WriteLine("{0} - {1}", "Title", data.Body);
        System.Console.WriteLine("{0} - {1}", "Tags", string.Join(", ", data.Tags));
        System.Console.WriteLine("{0} - {1}", "ID", data.Id);
        System.Console.WriteLine("{0} - {1}", "Site ID", data.SiteId);
        System.Console.WriteLine(data.Fetch);
        System.Console.WriteLine();

I'd rename the Console namespace to MyConsoleApp or whatever - anything but something that clashes with a class that's in the System namespace. Then the Console.WriteLine calls wouldn't need to be fully qualified:

        Console.WriteLine("{0} - {1}", "Title", data.Body);
        Console.WriteLine("{0} - {1}", "Tags", string.Join(", ", data.Tags));
        Console.WriteLine("{0} - {1}", "ID", data.Id);
        Console.WriteLine("{0} - {1}", "Site ID", data.SiteId);
        Console.WriteLine(data.Fetch);
        Console.WriteLine();

This class smells:

public abstract class Data
{
}

It seems to be used as a marker base class...

public sealed class NewestQuestionsByTagData : Data
public sealed class ActiveQuestionsData : Data

Based on usage, it seems Id could be moved from the derived types to the base class:

public abstract class Data
{
    [JsonProperty("id")]
    public string Id { get; internal set; }
}

If that doesn't make sense, then I'd make Data a marker interface instead, which is less surprising than an empty abstract class - and include XML comments to clarify:

/// <summary>
/// A marker interface that marks a type as a JSON response data type.
/// </summary>
public interface IData { /* empty */ }
\$\endgroup\$
  • \$\begingroup\$ Yeah, I'll change that to something other than Console. As to the marker class: that was indeed its usage and I'll probably switch over to an interface. In the lastest version it contains an enum which has to be overridden but that might as well be an interface. I will refrain from moving id to the interface though; I haven't looked at chat-events yet but I'm assuming that PostId will not be applicable there. \$\endgroup\$ – Jeroen Vannevel May 29 '14 at 14:45
  • \$\begingroup\$ I suppose 1970-1-1 is a magic value but I'll clarify it with a comment instead to signify it's the start of unix time. \$\endgroup\$ – Jeroen Vannevel May 29 '14 at 14:45
  • \$\begingroup\$ I'd really define it somewhere, you probably need to use it in multiple places, no? (i.stack.imgur.com/CsGXq.png) \$\endgroup\$ – Mathieu Guindon May 29 '14 at 14:47
  • 1
    \$\begingroup\$ I don't think I do; it's defined in a custom converter that I can use for each date field that I receive and I can't send responses myself. If the need comes up, I'll refactor it. \$\endgroup\$ – Jeroen Vannevel May 29 '14 at 14:51
  • \$\begingroup\$ Edited it out, you're right. \$\endgroup\$ – Mathieu Guindon May 29 '14 at 14:57

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.