2
\$\begingroup\$

I'm trying to improve this wrapper by replacing the Newtonsoft.Json with System.Text.Json in its BinanceWebSocketClient class and more specifically the HandleObjectMessage method. It is a message handler, which is supposed to parse the JSON results returned by the combined stream.

Combined stream events are wrapped as follows: {"stream":"","data":}

The stream field is telling us what the stream name is. For ex. !miniTicker@arr. And data is the actual data corresponding to the stream name.

The handler HandleObjectMessage must recognize the JSON returned and parse it to the corresponding stream name object. If it's a kline, it should parse it to the KlineResponse class and so on.

GitHub (original HandObjectMessage)

GitHub (original BookTickerResponse)

Here are my changes to System.Text.Json. This is what I want to be reviewed. There were developers who actually used switch like here instead of that abstraction.

public class BookTickerResponse : ResponseBase<BookTicker>
{
    internal static bool TryHandle(JsonDocument response, ISubject<BookTickerResponse?> subject)
    {
        var stream = response.RootElement.GetProperty("stream").GetString() ?? string.Empty;

        if (!stream.ToLower().EndsWith("@bookTicker"))
            return false;

        var parsed = response.ToObject<BookTickerResponse>();

        subject.OnNext(parsed);

        return true;
    }
}
public sealed class BinanceWebSocketClient : IDisposable
{
    ...

    private bool HandleObjectMessage(string msg)
    {
        using var response = JsonDocument.Parse(msg);

        // Add object handlers
        return
            AggregateTradeResponse.TryHandle(response, Streams.TradeBinSubject) ||
            TradeResponse.TryHandle(response, Streams.TradesSubject) ||
            MiniTickerResponse.TryHandle(response, Streams.MiniTickerSubject) ||
            BookTickerResponse.TryHandle(response, Streams.BookTickerSubject) ||
            KlineResponse.TryHandle(response, Streams.KlineSubject);
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ From what perspective are you looking for a review? \$\endgroup\$ Dec 10, 2021 at 16:04
  • \$\begingroup\$ @PEterCsala, thank you for your comment! I was looking at other codes on GitHub and they used switch instead of that abstraction of mine in HandleObjectMessage. Here is an example: github.com/JuliaZaborowska/TPUM/blob/…. In other words, the code review I want is only over HandleObjectMessage and TickerResponse's TryHandle \$\endgroup\$
    – nop
    Dec 10, 2021 at 18:08
  • 1
    \$\begingroup\$ I improved the question, so it's clearer \$\endgroup\$
    – nop
    Dec 10, 2021 at 19:10

1 Answer 1

2
\$\begingroup\$

TryHandle

  • First of all I would not trust the input parameters. I suggest to make sure that they are not null before you start accessing their members (like response.RootElement)
  • As far as I know the GetProperty throws KeyNotFoundException if a given key is not present
  • The GetString might throw an InvalidOperationException if the JsonElement's ValueKind is neither String nor Null
  • ToLower().EndsWith: Depending on how often this method is called, a complied regex might provide better performance
  • As far I know JsonDocument does not have neither ToObject method nor extension method. Is it a custom built solution, like this?

HandleObjectMessage

  • At the first glance the single return statement seems a bit weird
  • Let me show you an alternative where the actions and their relation are separated
var probes = new Func<bool>[]
{
    () =>  AggregateTradeResponse.TryHandle(response, Streams.TradeBinSubject),
    () => TradeResponse.TryHandle(response, Streams.TradesSubject),
    () => MiniTickerResponse.TryHandle(response, Streams.MiniTickerSubject),
    () => BookTickerResponse.TryHandle(response, Streams.BookTickerSubject),
    () => KlineResponse.TryHandle(response, Streams.KlineSubject)
};

return probes.Any(probe => probe());
  • So, here we have defined several probe methods
  • Then we iterate through them until we find one where the probe passes or until we run out of options
  • With this approach the return statement is simple and easy to understand.

UPDATE #1: avoid closure

In order to capture the response in a closure you can do the following:

var probes = new Func<JsonDocument, bool>[]
{
    (res) =>  AggregateTradeResponse.TryHandle(res, Streams.TradeBinSubject),
    (res) => TradeResponse.TryHandle(res, Streams.TradesSubject),
    (res) => MiniTickerResponse.TryHandle(res, Streams.MiniTickerSubject),
    (res) => BookTickerResponse.TryHandle(res, Streams.BookTickerSubject),
    (res) => KlineResponse.TryHandle(res, Streams.KlineSubject)
};

return probes.Any(probe => probe(response));
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you! That looks good! :) \$\endgroup\$
    – nop
    Dec 10, 2021 at 21:13

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.