Providing a simple test server for protocol testing

Question

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 < 2) { ShowUsage("Parameter-Count"); return; }
 if (!Int32.TryParse(args[0], out PORT)) { ShowUsage("<port>"); return; }
 if (!Directory.Exists(args[1])) { ShowUsage("<dir>"); return; }
 PORT = Int32.Parse(args[0]);
 DirSaveTo = args[1];
 // Setting up the Server on <127.0.0.1>:<PORT>
 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 (MemoryStream m = new MemoryStream())
 {
 // Reading picture size and picture from stream
 bytesLeftToRead = Convert.ToInt32(r.ReadInt64()); // Reading size as Int32
 // Little/ Big Endian - Consider !
 Console.WriteLine("Expected filesize is " + bytesLeftToRead.ToString("#,##0") + " Bytes");
 while (bytesLeftToRead > 0) // Reading the picture
 {
 buf = r.ReadBytes(Math.Min(BUF_SIZE, bytesLeftToRead));
 m.Write(buf, 0, buf.Length);
 bytesLeftToRead = bytesLeftToRead - buf.Length;
 }
 // Storing picture on HDD with a fancy filename created in the StringBuilder
 m.Seek(0L, SeekOrigin.Begin);
 b.Clear();
 b.Append((++cntPicutresReceived).ToString("0000"));
 b.Append("__");
 b.Append(rnd.Next(Int32.MaxValue).ToString("X"));
 b.Append(".bmp");
 using (FileStream f = new FileStream(Path.Combine(DirSaveTo, b.ToString()), FileMode.Create))
 {
 m.CopyTo(f);
 }
 Console.WriteLine("Image saved at '" + Path.Combine(DirSaveTo, b.ToString()) + "'");
 }
 }
 }
 private static void ShowUsage(String s)
 {
 Console.WriteLine("Problem with '" + s + "'");
 Console.WriteLine();
 Console.WriteLine("USAGE \t program.exe <port> <dir>");
 Console.WriteLine("<port> : Port to listen to when waiting for connections");
 Console.WriteLine("<dir> : Directory to store received pictures at. The directory must exist");
 Console.WriteLine();
 Console.WriteLine("Press [ENTER]");
 Console.ReadLine();
 }
 }
}

Answer 1

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<string> 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))
 {
 memoryStream.CopyTo(fileStream);
 }
 OnLog("Image saved at '" + Path.Combine(saveFilePath, fileName.ToString()) + "'");
 }
 }
 }
}

The new Main using the TestServer. The global variables a no longer necessary.

static void Main(string[] args)
{
 var port = 0;
 // Ensuring that required parameters have been provided
 if (args.Length < 2) { ShowUsage("Parameter-Count"); return; }
 if (!Int32.TryParse(args[0], out port)) { ShowUsage("<port>"); return; }
 if (!Directory.Exists(args[1])) { ShowUsage("<dir>"); return; }
 var dirSaveTo = args[1];
 var testServer = new TestServer(port)
 {
 Log = Console.WriteLine
 };
 testServer.Start(dirSaveTo);
}

• t3chb0t: @RobH, ok, fixed that one too.
• AlexR: I wouldn't go super-hardcore anti magic number and define three (!) ints for a simple buffer size. A comment should do the trick: const int bufSize = 10 * 1024 // 10 KiB Buffer Size
• Roland Illig: Even the comment is redundant. The original bufSize definition is already perfect.