• FEATURED

View more

View more

View more

### Image of the Day Submit

IOTD | Top Screenshots

### The latest, straight to your Inbox.

Subscribe to GameDev.net Direct to receive the latest updates and exclusive content.

# Quality of my code

Old topic!

Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

7 replies to this topic

### #1Xanather  Members

Posted 11 January 2013 - 04:03 AM

I haven't done much network programming lately so I decided to make a small program. I quickly made this small program within 2 days, it uses .net's TcpClient and TcpListener classes.

All the program does is transfer files to/from computers. Server mode of the program accepts files, while client mode of the program sends files.

I would like any experienced (networking) programmers to tell me what I should improve on.

Looking at the code now I would:

2. split the code up a bit mode, its mostly crammed into one method - how would I do this though?

Here are the 4 classes used in the program:

Program.cs...

//By xanather.
using System;

namespace Transfar
{
class Program
{
static void Main(string[] args)
{
Console.Title = "Transfar - Clean and Fast TCP File Transfers";
if (args.Length == 0)
{
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("This executable needs to be passed some arguments...");
Console.ForegroundColor = color;
ShowHelp();
Console.Write("Press any key to close this program... ");
return;
}
else
{
if (args[0] == "-server")
{
Server server;
server = new Server();
server.Start(args);
}
else if (args[0] == "-client")
{
Client client;
client = new Client();
client.Start(args);
}
else
{
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("Invalid argument(s). Here is the list of arguments...");
Console.ForegroundColor = color;
ShowHelp();
Console.Write("Press any key to close this program... ");
return;
}
}
}
static void ShowHelp()
{
Console.WriteLine("Available arguments (commands) for this application:");
Console.WriteLine("Argument\t\tDescription");
Console.WriteLine("-server [dir] [port]\n");
Console.WriteLine("\t\t\tStarts the application in server mode so the");
Console.WriteLine("\t\t\tTransfar clients. Port forwarding on the router");
Console.WriteLine("\t\t\tmay be required so that the server can listen for");
Console.WriteLine("\t\t\tinternet connections.\n");
Console.WriteLine("\t\t\tThe [dir] argument specifies the folder/directory");
Console.WriteLine("\t\t\tfrom clients. This argument is required.\n");
Console.WriteLine("\t\t\tThe [port] argument specifies the port in which the");
Console.WriteLine("\t\t\tserver should listen to for accepting Transfer client");
Console.WriteLine("\t\t\tconnections. Default port is 5655. This argument is");
Console.WriteLine("\t\t\toptional.\n");
Console.WriteLine("\t\t\tStarts the application in client mode so the");
Console.WriteLine("\t\t\tapplication can send and upload files to a Transfar");
Console.WriteLine("\t\t\tserver.\n");
Console.WriteLine("\t\t\tThe [address] argument specifies the server's IP");
Console.WriteLine("\t\t\tin which to connect to. This argument is required.\n");
Console.WriteLine("\t\t\tThe [file] argument specifies the file on disk");
Console.WriteLine("\t\t\tthe client should send/upload to the server.");
Console.WriteLine("\t\t\tThis argument is required.\n");
Console.WriteLine("\t\t\tThe [port] argument specifies which port the client");
Console.WriteLine("\t\t\tshould use to connect to Transfar servers of the same");
Console.WriteLine("\t\t\tport. Default port is 5655. This argument is optional.\n");
}
}
}


ArrayHelper.cs...

//By xanather
using System;

namespace Transfar
{
static class ArrayHelper
{
public static bool Equals(byte[] a, byte[] b)
{
if (a.Length != b.Length)
{
return false;
}
for (int i = 0; i < a.Length; i++)
{
if (a[i] != b[i])
{
return false;
}
}
return true;
}
}
}


Client.cs...

//By xanather
using System;
using System.IO;
using System.Net;
using System.Net.Sockets;
using System.Text;

namespace Transfar
{
class Client
{
public static readonly byte[] clientVerify = new byte[] { 6, 2, 5, 1, 255 };
public string srcFile;
public int port;
public void Start(string[] args)
{
if (args.Length > 1)
{
}
else
{
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("No IP address has been entered.");
Console.WriteLine("It is a required argument.");
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
//get file argument
if (args.Length > 2)
{
if (File.Exists(args[2]))
{
srcFile = args[2];
}
else
{
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("The file specified does not exist.");
Console.WriteLine("It is a required argument.");
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
}
else
{
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("No file location has been entered.");
Console.WriteLine("It is a required argument.");
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
if (args.Length > 3)
{
try
{
port = int.Parse(args[3]);
if (port < 0 || port > 65565)
{
throw new FormatException();
}
}
catch (FormatException)
{
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("Invalid port argument...");
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
}
else
{
port = Server.defaultPort;
}
//check the size of the file
try
{
long fileSize = checked(new FileInfo(srcFile).Length);
}
catch (OverflowException)
{
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("The file is too big! The maximum size supported is " + long.MaxValue + " bytes.");
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
ProcessConnection();
}
public void ProcessConnection()
{
TcpClient client;
NetworkStream stream;
try
{
Console.WriteLine("Attempting to connect...");
stream = client.GetStream();
}
catch (SocketException e)
{
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("Cannot connect to server.");
Console.WriteLine(e.Message);
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
Console.WriteLine("Connected to server.");
byte[] writeBuffer;
byte[] writeBuffer1;
//send client verification
stream.Write(clientVerify, 0, clientVerify.Length);
{
}
{
stream.Close();
client.Close();
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("Disconnected, invalid server.");
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
//write file name
string fileName = Path.GetFileName(srcFile);
writeBuffer1 = Encoding.UTF8.GetBytes(fileName);
writeBuffer = new byte[writeBuffer1.Length + 1];
writeBuffer[0] = (byte)writeBuffer1.Length;
Buffer.BlockCopy(writeBuffer1, 0, writeBuffer, 1, writeBuffer1.Length);
stream.Write(writeBuffer, 0, writeBuffer.Length);
{
stream.Close();
client.Close();
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("Disconnected, the file {0} already exists in the server directory.");
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
//write the size (in bytes) of the file
long fileSize = new FileInfo(srcFile).Length;
stream.Write(BitConverter.GetBytes(fileSize), 0, 8);
long writeRemaining = fileSize;
int percent = 0;
int prevPercent = -1;
using (FileStream fileStream = new FileStream(srcFile, FileMode.Open))
{
while (writeRemaining != 0)
{
readAmount = short.MaxValue; //write 65565 chuncks.
{
}
try
{
}
catch (IOException e)
{
stream.Close();
client.Close();
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("Disconnected: " + e.Message);
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
percent = (int)(((fileSize - writeRemaining) / (double)fileSize * 100));
if (percent != prevPercent)
{
prevPercent = percent;
Console.WriteLine("{0} - Upload at {1}%. {2}KB remaining ({3}KB/{4}KB).", fileName, percent, writeRemaining / 1024, (fileSize - writeRemaining) / 1024, fileSize / 1024);
}
}
}
//write clientVerify for final handshake
stream.Write(clientVerify, 0, clientVerify.Length);
stream.Close();
client.Close();
Console.Write("Press any key to close this program... ");
}
}
}


Server.cs...

//By xanather
using System;
using System.IO;
using System.Net;
using System.Net.Sockets;
using System.Text;

namespace Transfar
{
class Server
{
public static readonly byte[] serverVerify = new byte[]{10, 10, 35, 244, 21};
public const int defaultPort = 5655;
public int port;
public string destDirectory;
public AutoResetEvent connectionSignal = new AutoResetEvent(false);
public TcpListener listener;
public void Start(string[] args)
{
//get directory argument
if (args.Length > 1)
{
if (!args[1].EndsWith(Path.DirectorySeparatorChar.ToString()))
{
args[1] += Path.DirectorySeparatorChar;
}
if (Directory.Exists(args[1]))
{
destDirectory = args[1];
}
else
{
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("The directory specified does not exist.");
Console.WriteLine("It is a required argument.");
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
}
else
{
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("No destination directory has been entered.");
Console.WriteLine("It is a required argument.");
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
//get port argument
if (args.Length > 2)
{
try
{
port = int.Parse(args[2]);
if (port < 0 || port > 65565)
{
throw new FormatException();
}
}
catch (FormatException)
{
ConsoleColor color = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Cyan;
Console.WriteLine("Invalid port argument...");
Console.ForegroundColor = color;
Console.Write("Press any key to close this program... ");
return;
}
}
else
{
port = defaultPort;
}
StartListening();
}
public void StartListening()
{
listener.Start();
while (true)
{
Console.WriteLine("Listening for connections...");
ProcessClientAsync();
connectionSignal.WaitOne();
connectionSignal.Reset();
}
}
public async void ProcessClientAsync()
{
TcpClient client = await listener.AcceptTcpClientAsync();
NetworkStream stream = client.GetStream();
connectionSignal.Set();
//connection variables
int socketPort = int.Parse(((IPEndPoint)client.Client.RemoteEndPoint).Port.ToString());
//verify client connection
{
}
{
stream.Close();
client.Close();
Console.WriteLine(string.Format("[{0}:{1}] Warning: Invalid connection request by this IP.", socketAddress, socketPort));
return;
}
//send server verification
stream.Write(serverVerify, 0, serverVerify.Length);
{
}
//write if file already exists in destDirectory
if (File.Exists(destDirectory + fileName))
{
stream.Write(new byte[] { 0 }, 0, 1);
stream.Close();
client.Close();
Console.WriteLine(string.Format("[{0}:{1}] Disconnected, tried to save a existing file ({2}).", socketAddress, socketPort, fileName));
return;
}
else
{
stream.Write(new byte[] { 1 }, 0, 1);
}
//read the size (in bytes) of the file
{
}
int percent = 0;
int prevPercent = -1;
using (FileStream fileStream = new FileStream(destDirectory + fileName, FileMode.CreateNew))
{
{
{
}
{
try
{
}
catch (IOException e)
{
stream.Close();
client.Close();
Console.WriteLine(string.Format("[{0}:{1}] Disconnected: {2}", socketAddress, socketPort, e.Message));
Console.WriteLine(string.Format("[{0}:{1}] The file has not been received in full and may be corrupted.", socketAddress, socketPort, e.Message));
return;
}
}
percent = (int)(((fileSize - readRemaining) / (double)fileSize) * 100);
if (percent != prevPercent)
{
prevPercent = percent;
}
}
}
{
}
{
stream.Close();
client.Close();
Console.WriteLine(string.Format("[{0}:{1}] Warning: Unspecified error, file may not be received properly.", socketAddress, socketPort));
return;
}
else
{
}
stream.Close();
client.Close();
}
}
}


Ok, that ended up longer than I thought.

All replies are appriciated.

Thanks,

Xanather.

### #2KnolanCross  Members

Posted 11 January 2013 - 09:11 AM

The basics should be divide your code into methods and use private variables.

Currently working on a scene editor for ORX (http://orx-project.org), using kivy (http://kivy.org).

### #3Xanather  Members

Posted 11 January 2013 - 11:41 PM

Yeah I thought I should have used more methods, with the server class though having multiple methods for each stage of the connnection would mean I would need to keep passing references around (TcpClient/NetworkStream) between the methods for no reason?

I also find making everything public is easier to deal with, I could understand why you would do encapsulation in public libraries though?

### #4magicstix  Members

Posted 12 January 2013 - 12:14 AM

My pet peeve is not making it clear that variables are member variables, but that's more of a C++ism I guess than a C# one. The problem is someone reading your code will have a hard time telling what scope the variable is in without something like m_ or self. (as in the case of python) on the front of it. It's just a readability nitpick, but at work forgetting m_ or the less common using m_ for local scope variables is punishable by death. >:]

### #5DarkRonin  Members

Posted 31 January 2013 - 05:36 PM

My pet peeve is not making it clear that variables are member variables, but that's more of a C++ism I guess than a C# one. The problem is someone reading your code will have a hard time telling what scope the variable is in without something like m_ or self. (as in the case of python) on the front of it. It's just a readability nitpick, but at work forgetting m_ or the less common using m_ for local scope variables is punishable by death. >:]

Punishable by death?

The laws must be a bit more relaxed over here in Australia. LOL

Win32 Developer
One Of Them - Martial arts game that is mid development.

### #6frob  Moderators

Posted 01 February 2013 - 12:53 AM

Putting my code review hat on and reading through the code in the order you posted it.

Program.Main().

Make a function to read args first.

Make the output of your title and text blurb optional.  In the unix mentality you wouldn't show them at all unless the --version parameter was used.

Get rid of the "Press any key to close" message, or at least make it not there by default, unless an --interactive parameter was used.

Move the error messages to a separate function.  Instead of telling them the correct options, ask them to use the --help parameter.

The ArrayHelper.Equals function you included looks almost exactly like the Enumerable.SequenceEqual method.  Your usage is:

bool result = ArrayHelper.Equals(a, b);

The built in SequenceEqual method would be:

bool result = a.SequenceEqual(b);

Client.clientVerify array. I can see what you are trying to do with it, but WHY are you trying to use it?  I cannot see any benefit, more on that later.

Client.srcFile, .port, .address.  Why are these public?

Client.Start() method.

What's with all the colorful output?  This function has about 35 lines trying to output something that looks nice, when really all you need is a hard failure.  You can add an --interactive or --verbose flag that shows the pretty cyan if you think that is somehow helpful to your users.  Personally I find it a distraction.  Either succeed in silence or fail noisily; with bright red if you must use a color.

You throw a FormatException and then catch it two lines later.  Why?  Exceptions require significant resources and should be used for exceptional circumstances, not as a convenient way to break a loop.

Cleaning that up brings your 90 line function down to this:

            address = args[1];
srcFile = args[2];
port = Server.defaultPort;
if(args.Length>2) {
int tempVal;
port = int.TryParse(args[3], out tempVal) ? tempVal : Server.defaultPort;
}
if(ValidateParams())
ProcessConnection();


Much easier to read than 90 lines of console font coloring.

Client.ProcessConnection.

First thing I did when reviewing this one was to remove another 30 lines of font coloring and verbose output.  If you absolutely must have them, make sure they are off by default.  If you want a --verbose flag or --interactive flag, use that.

You send a "client verification".  It looks like just some magic number to indicate "hello".  Why not just send "hello" and get it over with? Why pick those specific bytes?  Do they buy you anything?

Rather than converting everything into byte arrays, use a StreamWriter object.  That's why they exist.

Just those changes alone drop your function down to around 70 lines of code.  I'd reduce it further, but you should get the idea at this point.

The rest of the code suffers from the same problems.

Just running over the rest of it briefly I dropped it to 1/3 of its original size.  That makes it easier to understand.  And that makes it less prone to bugs, and easier to fix those bugs when they are discovered.

Out of curiosity, why not use any of the bajillion existing file transfer tools out there?  Is this a learning experiment?  If not, I'd recommend going with SFTP.  It is a long-established secure protocol that can be called directly as a standalone executable or with a wrapper in your code.  Or if you want to go with .NET, use the Background Intellegent Transfer Service (BITS).  Using either of those directly is still a tiny fraction of the code you have written above.

If you mean this as a learning experience about networking, it looks like you have done all the basics.  As far as a learning experiment goes it appears to be functional, and that is often enough by itself.

Putting myself in a teaching role, I would put it in this type of category:  "You just wrote your own linked list class.  Congratulations, you understand linked lists.  Now that you know how they work, never write a linked list again if you can help it, you should always use the built-in classes."  You just learned the basics of how to transfer data.  Now that you know how and why it works, you have learned all you need.  From now on use a well-written, established, debugged, optimized library rather than wasting your time re-inventing the wheel.

To "rate the quality" of your code as you requested, it is clearly a beginner's code.  It would not survive a code review in a professional environment (one of us would sit down with the junior developer and help him fix the issues in an attempt to better his skills).

Check out my book, Game Development with Unity, aimed at beginners who want to build fun games fast.

Also check out my personal website at bryanwagstaff.com, where I occasionally write about assorted stuff.

### #7Xanather  Members

Posted 02 February 2013 - 10:58 AM

What's with all the colorful output?

Yea. looking back I would have never done this, your right it took up way to many lines.

The ArrayHelper.Equals function you included looks almost exactly like the Enumerable.SequenceEqual method. Your usage is:

bool result = ArrayHelper.Equals(a, b);

The built in SequenceEqual method would be:

bool result = a.SequenceEqual(b);

Didnt even know such thing existed, thanks (i should really look at the msdn docs more)

You throw a FormatException and then catch it two lines later. Why? Exceptions require significant resources and should be used for exceptional circumstances, not as a convenient way to break a loop.

I have also realized this recently, I am not doing this anymore.

You send a "client verification". It looks like just some magic number to indicate "hello". Why not just send "hello" and get it over with? Why pick those specific bytes?

I really thought it didn't matter, just a sequence of bytes for a "hello". Instead of converting hello into bytes at runtime why not just have a sequence of bytes?

Client.srcFile, .port, .address. Why are these public?

Good point, I don't worry about encapsulation that much (I would if I was developing a public library). I should start thinking about what proper accessibility I should mark when I am programming though.

### #8Xanather  Members

Posted 02 February 2013 - 11:00 AM

And yes, this was of course learning exercise for me. I never developed a file-transfer program before, decided to have a stab at it. Using a library would've defeated the whole purpose. My intention was not to re-invent the wheel.

Edited by Xanather, 02 February 2013 - 11:02 AM.

Old topic!

Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.