3
\$\begingroup\$

I'm writing this API that connects to my ASP.NET Core Cloud App and listens for potential Database changes from my CMS.

The code runs and works fine but I'm not sure if my code is considered "good" or bad.

I've tried to catch all potential Exceptions so that the user of the API can reconnect automaticly because there is no way to restart things manually.

Sorry for the amount of code I provided that's because people that read it can better understand the context.

Is there something that i can improve, or is my coding style a horror :)

Sorry for my English im not a native speaker.

    public BackendConnectionService(Uri serverUri, ClientType clientType, bool reconnect) {

        if (!IsValidUri(serverUri)) {
            throw new UriFormatException($"{nameof(serverUri)} is not a valid Uri!");
        }

        Reconnect = reconnect;
        _cloudUri = serverUri;
        Initialize(clientType);
        Connect();
    }

    public bool IsConnected => _webSocket.State == WebSocketState.Open;

    public bool Reconnect { get; set; }

    public event BackendChanged OnBackendChanged;
    public delegate void BackendChanged(BackendChangedEventArgs backend);

    public event BackendClosed OnBackendClosed;
    public delegate void BackendClosed();

    public event BackendError OnBackendError;
    public delegate void BackendError(Exception ex);

    public event BackendConnected OnBackendConnected;
    public delegate void BackendConnected();

    /// <summary>
    ///     Initializes Client & Networking
    /// </summary>
    /// <param name="clientType"></param>
    private void Initialize(ClientType clientType) {
        _client = new Client {
            ClientType = clientType
        };

        _webSocket = new ClientWebSocket();
        _cancellationTokenSource = new CancellationTokenSource(new TimeSpan(1, 0, 0, 0));
    }

    /// <summary>
    ///     Establishes Connection with Cloud App
    /// </summary>
    public async void Connect() {
        try {
            await _webSocket.ConnectAsync(_cloudUri, CancellationToken.None);
            await Task.Run(async () => {

                Console.WriteLine($"[{_timeStamp}]: Connected with Cloud App.");
                OnBackendConnected?.Invoke();

                await _webSocket.SendAsync(
                    PrepareMessage($"{nameof(CommandType.ClientRegistration)}{JsonConvert.SerializeObject(_client)}"),
                    WebSocketMessageType.Text, true, CancellationToken.None);

                ReadFromSocket();
            });
        }
        catch (Exception ex) {
            Console.WriteLine($"[{_timeStamp}]: Disconnected with Cloud App.");
            _webSocket = null;
            _cancellationTokenSource = null;
            OnBackendError?.Invoke(ex);

            if(Reconnect) {
                TryToReconnect();
            }
            else {
                throw new Exception(
                    "Could not connect try setting reconnect to true. And subscribe to OnBackendError");
            }
        }
    }

    private async void ReadFromSocket() {

        var receivedBytes = new byte[8192];
        var receivedBuffer = new ArraySegment<byte>(receivedBytes);

        try {
            while (true) {

                if (_webSocket.State == WebSocketState.Closed) {
                    OnBackendClosed?.Invoke();
                    break;
                }

                var socketResult = await _webSocket.ReceiveAsync(receivedBuffer, _cancellationTokenSource.Token);
                var bytes = receivedBuffer.Skip(receivedBuffer.Offset).Take(socketResult.Count).ToArray();
                var receivedMessage = Encoding.UTF8.GetString(bytes);

                if (string.IsNullOrEmpty(receivedMessage)) {
                    continue;
                }

                if (receivedMessage.StartsWith(nameof(ResponseType.ConnectedClientsResponse))) {
                    _connectedMediaSphereClientsData = receivedMessage;
                    return;
                }

                OnBackendChanged?.Invoke(new BackendChangedEventArgs {
                    JsonBackendData = receivedMessage,
                    ByteBackendData = bytes,
                    BackendChangedDateTime = DateTime.Now
                });
            }
        }
        catch (Exception ex) {
            Console.WriteLine($"[{_timeStamp}]: Disconnected with Cloud App.");
            OnBackendError?.Invoke(ex);

            if (Reconnect) {
                TryToReconnect();
            }
            else {
                throw new Exception(
                    "Could not connect try setting reconnect to true. And subscribe to OnBackendError");
            }
        }
    }

    public void Disconnect() {
        if (IsConnected) {
            _webSocket.CloseAsync(WebSocketCloseStatus.Empty, string.Empty, CancellationToken.None);
            Console.WriteLine($"[{_timeStamp}]: Disconnected with Cloud App.");
        }
    }

    /// <summary>
    ///     Pings the Backend
    /// </summary>
    public async Task PingAsync() {
        if (!IsConnected) {
            OnBackendClosed?.Invoke();
            return;
        }

        await Send("ping!");
    }

    /// <summary>
    ///     Sends data to the Backend
    /// </summary>
    /// <param name="data"></param>
    public async Task<bool> Send(string data) {
        if (!IsConnected) {
            OnBackendClosed?.Invoke();
            return false;
        }

        if (string.IsNullOrEmpty(data)) return false;

        try {
            await _webSocket.SendAsync(PrepareMessage(JsonConvert.SerializeObject(data)), WebSocketMessageType.Text,
                true, CancellationToken.None);
            return true;
        }
        catch (Exception) {
            Disconnect();
            return false;
        }
    }

    private static ArraySegment<byte> PrepareMessage(string data) {
        var encodedStr = Encoding.UTF8.GetBytes(data);
        return new ArraySegment<byte>(encodedStr, 0, encodedStr.Length);
    }

    private void TryToReconnect() {

        Console.WriteLine($"[{_timeStamp}]: Trying to reconnect....");
        _webSocket = new ClientWebSocket();
        _cancellationTokenSource = new CancellationTokenSource(new TimeSpan(1, 0, 0, 0));

        if(!Reconnect) {
            return;
        }
        Connect();
    }

    private bool IsValidUri(Uri serverUri) {
        return !string.IsNullOrEmpty(serverUri.OriginalString) && serverUri.OriginalString.Contains("ws");
    }
    #endregion
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Class

You have to implement IDisposable (assuming _webSocket type is derived from WebSocket then it implements IDisposable).

In your constructor you validate the URI and in IsValidUri() you use String.IsNullOrEmpty() however you can't create an Uri object with null (it will throw ArgumentNullException) or empty (it will throw ArgumentException) strings.

If Reconnect has a public setter then you should consider to remove the reconnect parameter from your ctor and I'd strongly suggest to do not perform connection in ctor. Complex operation in ctor are both a pain to debug and astonishing for the caller: constructor should crate an object in a consistent state, it should not perform any other task.

You should consider to name your async methods with the Async suffix.

Events and delegates

You currently have this:

public event BackendChanged OnBackendChanged;
public delegate void BackendChanged(BackendChangedEventArgs backend);

However you do not need to re-define a new delegate for each event (not to mention that inner classes - and delegates as extension - are often discouraged). You can use EventHandler<T>:

public event EventHandler<BackendChangedEventArgs> OnBackendChanged;

For OnBackendClosed similarly:

public event EventHandler OnBackendClosed;

Note that your event handler now consistently use EventHandler pattern for events (not a rule but a suggested design guideline). About guidelines also note that events should not have On prefix. BackendError has not its own *EventArgs class, introduce it:

public event EventHandler<BackendChangedEventArgs> BackendChanged;
public event EventHandler<BackendErrorEventArgs> BackendError;
public event EventHandler BackendConnected;
public event EventHandler BackendClosed;

Now we can take a look to their names. Do you need to repeat Backend* for each event when they're inside a BackendConnectionService? I think they can be simplified making your code both easier to read and less prolix.

I'd also consider to rename *Closed in Disconnected, Changed to DataReceived, and fix typo in Connectd:

public event EventHandler<BackendDataReceivedEventArgs> DataReceived;
public event EventHandler<BackendErrorEventArgs> Error;
public event EventHandler Connected;
public event EventHandler Disconnected;

Connection bugs

Currently if Connect() fails you call TryReconnect() which calls Connect() which if it will fail again will call TryReconnect() which...will end with a StackOverflowException.

Failed call to Connect() will leave the object in an invalid state during the invocation of Error. If event handler will check IsConnected property to determine if socket is connected then it will get a NullReferenceException because _webSocket is null.

Connection

You create a Task just to await it. Given that SendAsync is already awaitable then it makes no sense (unless your purpose is to make Connected event asynchronous. Not best option in general and especially in this case because connection might be destroyed while event handler is in execution or after/before that (calls to SendAsync and ReadFromSocket might fail and socket is then destroyed).

Disconnection

You call CloseAsync() but you do not wait for it. Why don't you make it async?

Shared considerations

You're catching Exception and that's a bad idea for a number of reasons (do you want to retry connection if you have AccessViolationException or OutOfMemoryException?) Carefully check which exceptions may be thrown and catch only them (in addition to AggregateException).

Do not throw Exception. Pick the most proper specific exception or write your own one but be specific or you will force callers to catch Exception (we do not like it in our code...don't force them to do it!) Also you may consider (I do not know security implications in your scenario) but you may want to keep reference of original exception:

throw new BackendException("...", ex);

OnBackendClosed (now Closed) is invoked when you attempt an operation after closing the backend. It's little bit astonishing, I'd invoke this event after connection has been closed or I'd rename it to something more meaningful (OperationOnClosedBackend?) Shouldn't it be an error? I'd expect an InvalidOperationException...

Your class is not complete (please do not post partial code) but please check that, if it's public, ClientType is a read-only property).

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for taking the time to show me that my code is far from good and basically show me the reality I will need time to fully understand your review. Awesome ! \$\endgroup\$
    – iNCEPTiON_
    Jun 2, 2017 at 9:45
  • \$\begingroup\$ Wait also for other reviews! Well, isn't it why we do code reviews? To make our code little bit better, step by step! \$\endgroup\$ Jun 2, 2017 at 9:46
  • \$\begingroup\$ Sure I will wait for other reviews! \$\endgroup\$
    – iNCEPTiON_
    Jun 2, 2017 at 9:49

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.