3
\$\begingroup\$

I'm using a third-party commercial server component to listen for SAP IDocs. I want to run the server inside a windows service and the vendor recommends having a separate method to check the server for a timeout and restart it if needed.

I created a wrapper for the server component and a class to check for timeouts periodically, but I think it could be done better. Specifically I think there is a lot of redundancy in the wrapper and am wondering if that is all needed, as well as what can be improved from the point of view of testability?

Any other feedback and observations would also be very welcome.

namespace IdocListener
{
    public interface IServer
    {
        bool IsRunning { get; }

        void Start();

        void Stop();

        event EventHandler<EventArgs> Started;

        event EventHandler<EventArgs> Stopped;

        event EventHandler<LoggedEventArgs> ServerMessage;

        event EventHandler<Exception> ServerException;
    }

    public class Server : IServer, IDisposable
    {
        bool isDisposed;

        public Server()
        {
            this.RfcServer = this.InitServer();

            this.RfcServerTimeoutChecker = new ServerTimeoutChecker(this);
        }

        RFCServer RfcServer { get; }

        ServerTimeoutChecker RfcServerTimeoutChecker { get; }

        public bool IsRunning
        {
            get { return this.RfcServer != null && this.RfcServer.IsRunning; }
        }

        /// <summary>
        /// Start the server
        /// </summary>
        public void Start()
        {
            if (!this.IsRunning)
            {
                this.RfcServer.Start();
            }
        }

        /// <summary>
        /// Stop the server
        /// </summary>
        public void Stop()
        {
            if (this.IsRunning)
            {
                this.RfcServer.Stop();
            }
        }

        public event EventHandler<EventArgs> Started;

        public event EventHandler<EventArgs> Stopped;

        public event EventHandler<LoggedEventArgs> ServerMessage;

        public event EventHandler<Exception> ServerException;

        RFCServer InitServer()
        {
            var rfcServer = new RFCServer
            {
                CanReceiveIdocs = true,
                Logging = true
            };

            rfcServer.Started += this.RfcServer_Started;
            rfcServer.Stopped += this.RfcServer_Stopped;
            rfcServer.Logged += this.RfcServer_Logged;
            rfcServer.InternalException += this.RfcServer_InternalException;
            rfcServer.IncomingIdoc += this.RfcServer_IncomingIdoc;

            return rfcServer;
        }

        // SERVER EVENTS

        void RfcServer_Started(object sender, EventArgs e)
        {
            this.Started?.Invoke(this, e);
        }

        void RfcServer_Stopped(object sender, EventArgs e)
        {
            this.Stopped?.Invoke(this, e);
        }

        void RfcServer_InternalException(RFCServer sender, Exception e)
        {
            string serverTypeName = this.RfcServer.GetType().Name;

            this.ServerException?.Invoke(this, new Exception($"The internal {serverTypeName} through an exception", e));
        }

        void RfcServer_Logged(object sender, LoggedEventArgs e)
        {
            this.ServerMessage?.Invoke(this, e);
        }

        void RfcServer_IncomingIdoc(RFCServer sender, Idoc idoc)
        {
            throw new NotImplementedException();
        }

        // IDISPOSABLE IMPLEMENTATION

        public void Dispose()
        {
            this.Dispose(true);

            GC.SuppressFinalize(this);
        }

        protected virtual void Dispose(bool disposing)
        {
            if (this.isDisposed || !disposing)
            {
                return;
            }

            this.RfcServer?.Dispose();

            this.isDisposed = true;
        }
    }

    public class ServerTimeoutChecker
    {
        readonly IServer server;

        readonly Timer timeoutChecker;

        public ServerTimeoutChecker(IServer server)
        {
            if (server == null)
            {
                throw new ArgumentNullException(nameof(server));
            }

            this.server = server;
            this.server.Started += this.Server_Started;
            this.server.Stopped += this.Server_Stopped;

            this.timeoutChecker = new Timer(30000D)
            {
                AutoReset = true,
                Enabled = true,
            };

            this.timeoutChecker.Elapsed += this.TimeoutChecker_Elapsed;
        }

        void Server_Started(object sender, EventArgs e)
        {
            this.timeoutChecker.Start();
        }

        void Server_Stopped(object sender, EventArgs e)
        {
            this.timeoutChecker.Stop();
        }

        void TimeoutChecker_Elapsed(object sender, ElapsedEventArgs e)
        {
            if (!this.server.IsRunning)
            {
                this.server.Start();
            }
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

Unclear Specification

The specification of the IServer component is not clear. Does it serve as an abstraction of the third-party RfcServer component (because it does not have its own interface?) or does it act as a generic interface for any kind of server (with or without automatic restart functionality?)?

Things I would immediately address:

  • proper documentation of the IServer component
  • a boolean flag get/set EnableAutomaticReconnect would make it clear that automatic reconnection is part of the interface for any kind of server implementation
  • If the third-party component has its own interface, use that instead of the class. This would also increase testiability because an interface is much easier to mock than a class.

This remaining part of the review asserts that:

  1. IServer component is an abstraction of the third-party component, since it lacks its own interface
  2. Automatic reconnection is part of the specification IServer, not of the particular implementation Server

Redundancy

Specifically I think there is a lot of redundancy in the wrapper and am wondering if that is all needed, ..

Since the interface is an abstraction of the class, I see no other way then to create wrapper members of all the public parts of the third-party class that you require. This design is called the Interface Segregation Principle. You then have to implement the Bridge Pattern to link the third-party implementation to your interface. IServer acts as the interface and Server as the bridge between IServer and RfcServer.

There is a minor inconsistency in your implementation. You expect this.RfcServer to may hold the value null; for instance in IsRunning -> return this.RfcServer != null && this.RfcServer.IsRunning; or in Dispose -> this.RfcServer?.Dispose() but not in Start or Stop -> this.RfcServer.Start(). Since you ensure it's never null you should avoid the null-checks altogether.

Testability

.. as well as what can be improved from the point of view of testability?

You have decoupled the IServer component from the decorator ServerTimeoutChecker. This is good practice because each of these classes server their specific purpose, which shows us the benefits of the Single Responsibility Principle. ServerTimeoutChecker can be tested with a mocked implementation of IServer rather than Server or even worse RfcServer. The only class harder to unit test is Server since it depends on the third-party component RfcServer. Unit-testing this class requires the use of a shim/fake rather than a mock. I would typically write integration and system integration tests with Server rather than unit tests.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you, that was helpful \$\endgroup\$ – yu_ominae Sep 18 at 20:07

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.