Writing a client wrapper class to isolate internal details of the client whose class implements IDisposable but interface does not

Question

I am writing a client wrapper around an external client that is defined in a NuGet package. The NuGet package contains below interface and the class.

public interface IServiceClient
{
    Task<Job> CreateJobAsync(JobDetails jobdetails);
}

public class ServiceClient : IServiceClient, IDisposable
{
    public async Task<Job> CreateJobAsync(JobDetails jobDetails)
    {   
        // Some processing and returns a job that contains required response and status

        return job;
    }
}

In my application, I write a client wrapper around the service client as below:

public interface IServiceClientWrapper
{
    Task<ResponseDto> PostAsync(RequestDto request);
}

public class ServiceClientWrapper : IServiceClientWrapper
{
    private static IServiceClient serviceClient;

    public static void Init()
    {
        // See below for defintion
        serviceClient = ClientFactory.Create();
    }   

    public async Task<ResponseDto> PostAsync(RequestDto request)
    {
        // Convert request to JobDetails as required

        var job = await serviceClient.CreateJobAsync(jobDetails);

        // Convert job to ResponseDto and return
        return response;
    }

    // Since ServiceClient implements IDisposable
    public static void Close()
    {
        if (serviceClient != null)
        {
            ((ServiceClient)serviceClient).Dispose();
        }
    }
}

internal static class ClientFactory
{   
    public static IServiceClient ServiceClient { get; set; }

    public static IServiceClient Create()
    {
        if (ServiceClient != null)
        {
            // Used during unit testing
            return ServiceClient;
        }

        return new ServiceClient(APIBaseAddress, AccessKey);
    }
}

Questions:

Since the interface isn't marked IDisposable, I introduce Init and Close methods to do that. Is there a better way to handle this?
Having the serviceClient as static, is it thread-safe since I always invoke the non-static CreateJobAsync method with new paramters for each request?

Why wouldnt you just make `ServiceClientWrapper` implement `IDisposable` — maccettura, Mar 05 '18 at 21:07
Because during dispose I would need to cast the client to right object to call its dispose method. — vrcks, Mar 05 '18 at 21:12
@vwm are you not just doing that now on your `Close()` method? — maccettura, Mar 05 '18 at 21:18
Making it implement `IDisposable` will allow you to use `using` statements, plus its already the interface to use for such situations. Dont reinvent the wheel — maccettura, Mar 05 '18 at 21:18
With Close I can choose to ignore it or call whenever I want during unit testing. With IDisposable, I wouldn't have that control. Please correct me if my understanding it not right. — vrcks, Mar 05 '18 at 21:34

score 1 · Answer 1 · answered Mar 05 '18 at 21:12

1

I'd suggest that IDisposable is a detail of the concrete implementation of IServiceClientWrapper i.e. ServiceClientWrapper

It is not necessarily apart of the IServiceClientWrapper, as you may in theory have implementations which don't need to dispose of anything.

So as the comments suggest, ServiceClientWrapper should implement IDisposable.

answered Mar 05 '18 at 21:12

Alex KeySmith

16,657
11
74
152

But the IServiceClient inside the ServiceClientWrapper is static and needs to be alive through the lifetime of the application. Is IDisposable preferable in such a case? – vrcks Mar 07 '18 at 19:27
Ah interesting, I didn't fully take on board the broader implications. I'd suggest perhaps your dependency framework should instead be managing the lifetime of your objects to avoid a consumer of ServiceClientWrapper disposing IServiceClient for everyone else. When I get some more time I'll try and edit my answer with more detail. I presume you're wrapper is there for unit testing? And that you want service client to be static to re-used throughout your app? – Alex KeySmith Mar 08 '18 at 09:33
Yes I am writing the wrapper to restrict the knowledge of ServiceClient to the wrapper class alone. Other parts of my application only know about the wrapper. They aren't aware it is using ServiceClient inside. Since ServiceClient is thread-safe, I want to utilize that fact and restrict its instantiation once. This is a legacy ocde and we don't have DI framework in place. Hence all this workaround. – vrcks Mar 12 '18 at 22:28

Writing a client wrapper class to isolate internal details of the client whose class implements IDisposable but interface does not

1 Answers1