1
\$\begingroup\$

I'm implementing an HTTP client shell that's going to be used for the lifetime of my application in C#. It needs to be initialized before the instance can be accessed. It looks something like this:

public static class AssetSyncServiceClient {
    private static object _lock = new object();
    private static bool _initialized = false;
    private static HttpClient _client = null;

    /// <summary>
    /// Indicates whether the client has been initialized.
    /// </summary>
    /// <returns>Whether the client is initialized.</returns>
    public static bool IsInitialized() {
        lock (_lock) {
            return _initialized;
        }
    }

    /// <summary>
    /// Creates and configures HTTP client.
    /// </summary>
    public static void Initialize(string baseUrl, bool onlyRequestJson = true) {
        lock (_lock) {
            if (_initialized) { throw new Exception("Client has already been initialized."); }

            _client = new HttpClient();
            _client.BaseAddress = new Uri(baseUrl);

            if (onlyRequestJson) {
                _client.DefaultRequestHeaders.Accept.Clear();
                _client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
            }

            _initialized = true;
        }
    }

    /// <summary>
    /// Closes / disposes of the HTTP client.
    /// </summary>
    public static void Close() {
        lock (_lock) {
            if (_initialized && _client != null) {
                _client.Dispose();
            }
        }
    }

    /// <summary>
    /// Gets client instance.
    /// </summary>
    public static HttpClient Instance {
        get {
            lock (_lock) {
                if (!_initialized) { throw new Exception("HTTP client has not been initialized."); }
                return _client;
            }
        }
    }
}

The trouble is, this throws up a warning in Visual Studio because of style rule CA1065 - Microsoft say you shouldn't throw an exception in a property. I just don't agree with this - it seems fine to me. If you try to access .Instance before it's initialized you get an exception. It's clearly a property whose code could throw an exception, and not a "dumb field". I could turn Instance into a GetInstance method but I just prefer the look of accessing it from calling code as a property.

So does it make sense to just suppress the warning here, or am I designing this badly? Is there a better way? Should I bite the bullet and make Instance a method?

UPDATE:
It looks like I probably want something other than a singleton because singletons are hard to test (although actually MS Fakes can mock static methods so maybe it's OK), so what would be a better design pattern for this functionality that's unit testable?

\$\endgroup\$
2
\$\begingroup\$
  • The initialize dance is not nice. Requires the caller to keep track of if it has been called etc. Pit of failure.
  • The IsInitialized() is not very useful as it is now as there no way to lock over check and initialize. There will always be a risk that another thread calls Initialize between the check IsInitialized and the call to Initialize.
  • I have not used HttpClient much but my gut feeling is that the locking is not needed.
  • A singleton will make it hard to mock in tests. What about writing it like this:
public class AssetSyncServiceClientSettings
{
    public string BaseUrl { get; set; }
    public bool JsonOnly { get; set; }
}

public interface IAssetSyncServiceClient : IDisposable
{
    // Picked a sample method randomly, you can expose an api that makes sense in your app/lib
    Task<string> GetStringAsync(string requestUri);
}

public sealed class AssetSyncServiceClient : IAssetSyncServiceClient
{
    private readonly HttpClient _client;
    private bool _disposed;

    public AssetSyncServiceClient(AssetSyncServiceClientSettings settings)
    {
        _client = new HttpClient();
        _client.BaseAddress = new Uri(settings.BaseUrl);

        if (settings.JsonOnly)
        {
            _client.DefaultRequestHeaders.Accept.Clear();
            _client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
        }
    }

    public Task<string> GetStringAsync(string requestUri)
    {
        VerifyNotDisposed();
        return _client.GetStringAsync(requestUri);
    }

    public void Dispose()
    {
        if (_disposed)
        {
            return;
        }

        _disposed = true;
        _client.Dispose();
    }

    private void VerifyNotDisposed()
    {
        if (_disposed)
        {
            throw new ObjectDisposedException(this.GetType().FullName);
        }
    }
}
  • Doing it like this avoids initialization. If you need it lazy for some reason the service could handle it internally.
  • An interface probably makes sense for this so you can mock it in tests.
  • Use the Dependency Injection container to make the service a singleton instance.
  • Pass in the service interface in to constructors where it is needed. It makes the dependency explicit and makes it easy to mock in tests.
  • The settings object can perhaps contain urls so that the service can expose methods like public async Task<Stuff> GetStuffAsync()
\$\endgroup\$
  • \$\begingroup\$ Good answer. One point I don't quite understand is "Pass in the service interface in to constructors where it is needed." - is that just a general principle, or are you referring to somewhere you have done this in your example code? \$\endgroup\$ – Jez Mar 12 '16 at 11:13
  • \$\begingroup\$ It is a general principle, constructor injection, not shown in the sample code. Or in a way it is, we pass in AssetSyncServiceClientSettings in the constructor of the client. Pass in IAssetSyncServiceClient the same way where the service is used. \$\endgroup\$ – Johan Larsson Mar 12 '16 at 11:59
  • \$\begingroup\$ One way I'm thinking of making this a singleton (without the complexity of a full Dependency Injection framework) is by using a "singleton factory"; a factory that creates/gets only one instance of an IAssetSyncServiceClient. Does that sound like a decent compromise? \$\endgroup\$ – Jez Mar 12 '16 at 14:42
  • \$\begingroup\$ Could be but you want it written in a way so that it can return a mock in tests I think. IoC frameworks are not hard, I can walk you through it later when I get home. Imo no app is too small for IoC. Ping me in chat in a couple of hours. \$\endgroup\$ – Johan Larsson Mar 12 '16 at 14:45
  • \$\begingroup\$ Which chatroom? \$\endgroup\$ – Jez Mar 12 '16 at 14:46

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.