HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

TCPListener server to discover clients on a network

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
clientstcplistenerdiscoverservernetwork

Problem

I am currently writing a program to sync files (music, photos, etc) from my PC to an Android device. In order to do this, I have 2 application: one that is running on my PC, and one that shall be running on the Android device. When the Android app will open, it will try to connect to a specified IP:port on the PC application, which has a server just listening for requests in order to show the clients in a WPF list style view. This server is written in F#.

Here is the server implementation I have come up with:

```
type TCPListenerServer(discoveryPort:int) =
let server = new TcpListener (IPAddress.Loopback, discoveryPort)

let activeConnections = new List()

let serverLoop () =
let rec loop (pendingConnection:Task) = async {
let newPendingConnection, client =
match pendingConnection.Status with
| TaskStatus.Created | TaskStatus.WaitingForActivation | TaskStatus.WaitingToRun | TaskStatus.Running ->
(None, None)
| TaskStatus.Faulted | TaskStatus.Canceled ->
raise (new System.NotImplementedException())
| TaskStatus.RanToCompletion ->
let connectionTask = server.AcceptTcpClientAsync ()
(Some connectionTask, Some pendingConnection.Result)
| _ ->
raise (new System.NotImplementedException())

// Add the new client to the list
Option.iter (fun c -> activeConnections.Add c) client

// Switch the new pending connection if there is one
let connectionAttempt = defaultArg newPendingConnection pendingConnection

Async.Sleep 1000 |> Async.RunSynchronously
return! loop connectionAttempt
}

try
server.Start ()

let connectionTask = server.AcceptTcpClientAsync ()

loop connectionTask
finally
server.Stop ()

member x.

Solution

I would consider altering member x.ActiveConnections for the reasons I mention on the answer here: Create database connection and run the insert, delete, update queries class

Basically, consumers of your class can actually modify the ActiveConnections list (call .Remove, etc.), so you should find a way to eliminate that.

In F#, I would probably do the following:

member x.ActiveConnections = activeConnections.ToArray()


Or as Dan Lyons stated in comments:

member x.ActiveConnections = activeConnections.AsReadOnly()


This guarantees that you are exposing a non-modifiable copy of the list to the consumer of your class. You could optionally open System.Linq and use:

member x.ActiveConnections = activeConnections.ToArray().AsEnumerable()


Both of these have the same effect: the internal List isn't able to be modified.

I also noticed you use x. as the prefix for all your member declarations, I'm not sure what the standard is (quick google pulled nothing) but I always try to use member this.Name for public-facing members, to remind myself that they're part of the same object. (F# doesn't seem to care much, but it still feels right.)

Code Snippets

member x.ActiveConnections = activeConnections.ToArray()
member x.ActiveConnections = activeConnections.AsReadOnly()
member x.ActiveConnections = activeConnections.ToArray().AsEnumerable()

Context

StackExchange Code Review Q#136791, answer score: 2

Revisions (0)

No revisions yet.