0

So, I got a question here that appears to be opinion based. Rather than tackling how and where I need to implement IDisposable, I'd rather question myself how the code is written, is it a good design.

I got a base Node class which has a StreamReader and StreamWriter and 2 derived classes Client and Server. The Client only really has to set up his side of the AnonymousPipes based upon the PipeHandles he gets from the server. The Server has to do more since he needs to be able to broadcast his PipeHandles.

In reality, the Main application (Server) starts the Child application (Client) with the PipeHandles as arguments.

I think my setup is pretty good. I've got an open base Node class which can simply Read and Write Streams. The derived classes have some more specific info they need to be initialized. Everything looks unit-testable. The "not so nice thing" I did in my opinion is how the Server class gets back his PipeStreams to get the handles. In order to do this the Node has a little bit too much info in order to be a base class (PipeStream properties).

Question: Would how I did it be a good way to do it if this was requested to program as a developer at a company? I'm not sure if the Client and Server constructors are executed well.

public class Node
{
    private readonly StreamReader streamIn;
    private readonly StreamWriter streamOut;

    protected Node(PipeStream pipeStreamIn, PipeStream pipeStreamOut)
    {
        PipeStreamIn = pipeStreamIn ?? throw new ArgumentNullException(nameof(streamIn));
        PipeStreamOut = pipeStreamOut ?? throw new ArgumentNullException(nameof(streamOut));

        streamIn = new StreamReader(pipeStreamIn);
        streamOut = new StreamWriter(pipeStreamOut)
        {
            AutoFlush = true
        };
    }

    protected PipeStream PipeStreamIn { get; }
    protected PipeStream PipeStreamOut { get; }

    public string Read()
    {
        return streamIn.ReadLine();
    }

    public void Write(string line)
    {
        streamOut.WriteLine(line);
    }
}

public class Client : Node
{
    public Client(string pipeInHandleAsString, string pipeOutHandleAsString) : base(new AnonymousPipeClientStream(PipeDirection.In, pipeInHandleAsString), new AnonymousPipeClientStream(PipeDirection.Out, pipeOutHandleAsString)) { }

    public void Sync()
    {
        // Sync code
    }
}

public class Server : Node
{
    public Server() : base(new AnonymousPipeServerStream(PipeDirection.In, HandleInheritability.Inheritable), new AnonymousPipeServerStream(PipeDirection.Out, HandleInheritability.Inheritable))
    {
        ClientPipeInHandleAsString = ((AnonymousPipeServerStream)PipeStreamOut).GetClientHandleAsString();
        ClientPipeOutHandleAsString = ((AnonymousPipeServerStream)PipeStreamIn).GetClientHandleAsString();
    }

    public string ClientPipeInHandleAsString { get; private set; }
    public string ClientPipeOutHandleAsString { get; private set; }

    public void Sync()
    {
        ((AnonymousPipeServerStream)PipeStreamIn).DisposeLocalCopyOfClientHandle();
        ((AnonymousPipeServerStream)PipeStreamOut).DisposeLocalCopyOfClientHandle();

        // Sync code
    }
}

I've also got my 1st code design below. There a protected Initialize method does the things the Node constructor does in my 2nd design. The Client makes his PipeStreams and passes them to the Node. The Server does something similar but needs them as a field since he needs to do something with them during the Sync.

public class Node
{
    private readonly StreamReader streamIn;
    private readonly StreamWriter streamOut;

    protected Initialize(PipeStream pipeStreamIn, PipeStream pipeStreamOut)
    {
        if (streamIn == null) throw new ArgumentNullException(nameof(streamIn));
        if (streamOut == null) throw new ArgumentNullException(nameof(streamOut));

        streamIn = new StreamReader(pipeStreamIn);
        streamOut = new StreamWriter(pipeStreamOut)
        {
            AutoFlush = true
        };
    }

    public string Read()
    {
        return streamIn.ReadLine();
    }

    public void Write(string line)
    {
        streamOut.WriteLine(line);
    }
}

public class Client : Node
{
    public Client(string pipeInHandleAsString, string pipeOutHandleAsString)
    {
        var streamIn = new AnonymousPipeClientStream(PipeDirection.In, pipeInHandleAsString);
        var StreamOut = new AnonymousPipeClientStream(PipeDirection.Out, pipeOutHandleAsString);

        Initialize(streamIn, StreamOut);
    }

    public void Sync()
    {
        // Sync code
    }
}

public class Server : Node
{
    private readonly AnonymousPipeServerStream streamIn;
    private readonly AnonymousPipeServerStream streamOut;

    public Server()
    {
        streamIn = new AnonymousPipeServerStream(PipeDirection.In, HandleInheritability.Inheritable);
        streamOut = new AnonymousPipeServerStream(PipeDirection.Out, HandleInheritability.Inheritable);

        ClientPipeInHandleAsString = streamOut.GetClientHandleAsString();
        ClientPipeOutHandleAsString = streamIn.GetClientHandleAsString();

        Initialize(streamIn, streamOut);
    }

    public string ClientPipeInHandleAsString { get; private set; }
    public string ClientPipeOutHandleAsString { get; private set; }

    public void Sync()
    {
        streamIn.DisposeLocalCopyOfClientHandle();
        streamOut.DisposeLocalCopyOfClientHandle();

        // Sync code
    }
}

Secondly, I think how and where to implement IDisposable is better fit for a separate question but I think I first need to decide which of the 2 above approaches the industry would rather have.


Update 1

So I've taken Kain0_0's comments into account and this is what I assume might be better. Node is renamed to Peer and Client and Server to ClientPeer and ServerPeer. Their backbone is the same, just how they set up the communication between the 2 is different. I wouldn't know how to enable both to start and listen to init communicaitons.

public class Peer
{
    private readonly StreamReader streamIn;
    private readonly StreamWriter streamOut;

    protected Node(PipeStream pipeStreamIn, PipeStream pipeStreamOut)
    {
        if (pipeStreamIn == null) throw new ArgumentNullException(nameof(streamIn));
        if (pipeStreamIn.CanRead) throw new ArgumentException($"{nameof(streamIn)} does not support reading.");
        if (pipeStreamOut == null) throw new ArgumentNullException(nameof(streamOut));
        if (pipeStreamOut.CanWrite) throw new ArgumentException($"{nameof(streamOut)} does not support writing.");

        streamIn = new StreamReader(pipeStreamIn);
        streamOut = new StreamWriter(pipeStreamOut)
        {
            AutoFlush = true
        };
    }

    public string Read()
    {
        return streamIn.ReadLine();
    }

    public void Write(string line)
    {
        streamOut.WriteLine(line);
    }
}

public class ClientPeer
{
    private readonly Peer peer;

    public ClientPeer(string pipeInHandleAsString, string pipeOutHandleAsString)
    {
        var pipeStreamIn = new AnonymousPipeClientStream(PipeDirection.In, pipeInHandleAsString);
        var pipeStreamOut = new AnonymousPipeClientStream(PipeDirection.Out, pipeOutHandleAsString);

        peer = new Peer(pipeStreamIn, pipeStreamOut);
    }

    public int MessagesSent => peer.MessagesSent;
    public int MessagesReceived => peer.MessagesReceived;

    public void Sync()
    {
        while (Read().Function != function) { }
        Write(new Message(Function.Sync, "From Client"));
    }

    public IMessage Read()
    {
        return peer.Read();
    }

    public string Write(IMessage message)
    {
        return peer.Write(message);
    }    
}

public class ServerPeer : IDisposable
{
    private readonly AnonymousPipeServerStream pipeStreamIn;
    private readonly AnonymousPipeServerStream pipeStreamOut;
    private readonly Peer peer;

    public ServerPeer()
    {
        pipeStreamIn = new AnonymousPipeServerStream(PipeDirection.In, HandleInheritability.Inheritable);
        pipeStreamOut = new AnonymousPipeServerStream(PipeDirection.Out, HandleInheritability.Inheritable);
        ClientPipeInHandleAsString = pipeStreamOut.GetClientHandleAsString();
        ClientPipeOutHandleAsString = pipeStreamIn.GetClientHandleAsString();

        peer = new Peer(pipeStreamIn, pipeStreamOut);
    }

    public string ClientPipeInHandleAsString { get; }
    public string ClientPipeOutHandleAsString { get; }
    public int MessagesSent => peer.MessagesSent;
    public int MessagesReceived => peer.MessagesReceived;

    public void Sync()
    {
        pipeStreamIn.DisposeLocalCopyOfClientHandle();
        pipeStreamOut.DisposeLocalCopyOfClientHandle();

        Write(new Message(Function.Sync, "From Server"));
        while (Read().Function != function) { }
    }

    public IMessage Read()
    {
        return peer.Read();
    }

    public string Write(IMessage message)
    {
        return peer.Write(message);
    }
}
  • Is there a reason to treat Client and Server identically anywhere? – Kain0_0 Jun 9 at 23:48
  • @Kain0_0, how do you mean? – Krowi Jun 10 at 6:12
  • Is there a function, an array, or variable that holds Node objects, and only interacts with them through the Node interface? Because if there isn't then I would not have Client and Server share such a base class. – Kain0_0 Jun 10 at 6:36
  • @Kain0_0, no. The Node is simply there to have shared methods for Client and Server. If you wouldn't share a base class in that case, wouldn't I go against the DRY principle of having identical Read and Write methods in both Client and Server? – Krowi Jun 10 at 6:49
  • 1
    I would call this an overzealous application of DRY. You are sacrificing meaning to follow a rule. In which way would a Read from a Server, and a Read from the Client be anyway identical? They have different meanings, which will force them to change differently, even if they currently contain identical implementations. Unless they are identical, and are in fact Peers, and then this collapses to a single class anyway. A good rule of thumb for using DRY is to count 3 to 5 repetitions before refactoring. Also it is better to use composition over inheritance for shared implementations. – Kain0_0 Jun 10 at 7:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.