Providing a simple test server for protocol testing

For an outsourced job (implementation of a network client) I will provide a simple server for protocol-testing. This is the code.

Do you consider this code readable/ comprehensible?

```
using System;
using System.Text;
using System.IO;
using System.Net.Sockets;

namespace SimplePictureReceiverServer
{
class Program
{
private static Int32 PORT = -1;
private static String DirSaveTo = null;

static void Main(string[] args)
{
// Ensuring that required parameters have been provided
if (args.Length "); return; }
if (!Directory.Exists(args[1])) { ShowUsage(""); return; }

PORT = Int32.Parse(args[0]);
DirSaveTo = args[1];

// Setting up the Server on :
TcpClient c = null; NetworkStream s;
TcpListener l = new TcpListener(System.Net.IPAddress.Loopback, PORT);
l.Start();

while(true)
{
// Waiting for a Client to connect
Console.Write("Waiting ... ");
c = l.AcceptTcpClient();
s = c.GetStream();
Console.WriteLine("Ok");

// Handle the client
try
{
using (BinaryReader r = new BinaryReader(s))
{
HandleClient(r);
}
}
catch(Exception e)
{
Console.WriteLine("Client disconnected (" + e.Message + ")");
Console.WriteLine();
}
}
}

private static void HandleClient(BinaryReader r)
{
const Int32 BUF_SIZE = 10 * 1024;
Int32 cntPicutresReceived = 0; StringBuilder b = new StringBuilder();

Random rnd = new Random();
Byte[] buf;

Int32 bytesLeftToRead;

while (true)
{
using (

Issues

Variable definitions

TcpClient c = null; 
NetworkStream s;

Use full names if it's not something obvious and really short like a LINQ query. Single letters are hard to understand.

Define your variables as close to their usage as possible.

Streams & IDisposable

tcpClient.GetStream()

If you see a stream always check the documentation. Most probably it's something disposable, like here.

But not only streams. Also network related object are mostly disposable so is the TcpClient.

Single Responsibility Principle

Always keep in mind that each class, method etc should be responsible for only one thing.

This means that your test server shouldn't be printing anything to the console. It may log something but let it log to an abstraction so that you can attach any logger to it. Now the TestServer no longer depends on the Console.

Random file names

Instead of creating random file names by yourself use the method already provided by .NET: Path.GetRandomFileName


The GetRandomFileName method returns a cryptographically strong, random string that can be used as either a folder name or a file name.

Encapsulation

Try to encapsulate the logic in appropriate modules rather then creating a bunch of static methods. It's easier to test and who knows, maybe you'll need it later somewhere else so if you do it right the first time, it'll be copy/paste the next time.

Parameters

Use (overloaded) construtors where appropriate to create the modules and initialize them to a correct state.

Magic numbers

const int bufferSize = 10 * 1024;

They are always bad. Even if something appears to be obvious now, it won't probably be that easy to understand in few weeks.

const int oneKilobyte = 1024;
const int kilobyteCount = 10;
const int bufferSize = kilobyteCount * oneKilobyte;

Exception.Message

"Client disconnected (" + e.Message + ")"

In most cases this is not enougth because there might be innter exceptions. e.ToString() is much more usefull.

The new TestServer.cs

With all the above applied the test server can be really pretty and reusable:

```
class TestServer
{
public Action Log { get; set; }

private void OnLog(string message)
{
Log?.Invoke(message);
}

private TestServer(IPAddress ip, int port)
{
IP = ip;
Port = port;
}

public TestServer(string ip, int port) : this(IPAddress.Parse(ip), port) { }

public TestServer(int port) : this(IPAddress.Loopback, port) { }

public IPAddress IP { get; }

public int Port { get; }

public void Start(string saveFilePath)
{
var tcpListener = new TcpListener(IP, Port);
tcpListener.Start();

while (true)
{
OnLog("Waiting ... ");
using (var tcpClient = tcpListener.AcceptTcpClient())
using (var networkStream = tcpClient.GetStream())
{
OnLog("Ok");

// Handle the client
try
{
using (var binaryReader = new BinaryReader(networkStream))
{
HandleClient(binaryReader, saveFilePath);
}
}
catch (Exception e)
{
OnLog("Client disconnected.");
OnLog(e.ToString());
}
}
}
}

private void HandleClient(BinaryReader binaryReader, string saveFilePath)
{
const int oneKilobyte = 1024;
const int kilobyteCount = 10;
const int bufferSize = byteCount * oneKilobyte;
var receivedPictureCount = 0;

while (true)
{
using (var memoryStream = new MemoryStream())
{
// Reading picture size and picture from stream
// Reading size as Int32
// Little/ Big Endian - Consider !
var bytesLeftToRead = Convert.ToInt32(binaryReader.ReadInt64());

OnLog("Expected filesize is " + bytesLeftToRead.ToString("#,##0") + " Bytes");

// Reading the picture
while (bytesLeftToRead > 0)
{
var buf = binaryReader.ReadBytes(Math.Min(bufferSize, bytesLeftToRead));
memoryStream.Write(buf, 0, buf.Length);
bytesLeftToRead = bytesLeftToRead - buf.Length;
}

// Storing picture on HDD with a fancy filename created in the StringBuilder
memoryStream.Seek(0L, SeekOrigin.Begin);

var fileName = new StringBuilder()
.Append((++receivedPictureCount).ToString("0000"))
.Append("__")
.Append(Path.GetRandomFileName())
.Append(".bmp");

using (var fileStream = new FileStream(Path.Combine(saveFilePath, fileName.ToString()), FileMode.Create))

StackExchange Code Review Q#145343, answer score: 4