File transfer over a stream

I am doing some programming on the side and would just like to know if I'm going in the right direction with my code.

```
public class FileTransfer
{
#region Variables
private const int MAX_CHUNK_SIZE = (1024 * 5000);
private static BinaryWriter fileWriter;
private static FileStream fileReader;
private static List files;
private static NetworkStream clientStream;
private static byte[] fileData;
private static UInt32 dataLength;
#endregion

#region Functions

///
/// Default Constructor Initializes a new list of files
/// and a new byte array set to the max chunk size
///
public FileTransfer()
{
files = new List();
fileData = new byte[MAX_CHUNK_SIZE];
}

///
/// Uses the Given File Stream to receive a file
///
/// Returns a bool depending on if the file was succesfully received or not.
public bool ReceiveFile(TcpClient client, string saveLocation = @"Files")
{
clientStream = client.GetStream();
string fileName = GetFileName();
dataLength = GetFileLength();

OpenSaveFilePath(saveLocation + "/" + fileName);

while (dataLength != 0 && client.Connected)
{
int bytesRead = GetNextChunk();
if (bytesRead != 0)
WriteChunkToFile(bytesRead);

}
fileWriter.Close();
return true;
}

///
/// Writes the read file chunk to the opened file
///
/// number of bytes read that chunk
private static void WriteChunkToFile(int bytesRead)
{
fileWriter.Write(fileData, 0, bytesRead);
dataLength -= (UInt32)bytesRead;
SetByteArray(out fileData, dataLength);
}

///
/// Gets the next chunk of data from the file
///
/// Returns the total number of bytes read this chunk
private static int GetNextChunk()
{
int byteTotal = 0;
do
{
int d

Why all these private static members? Instead of making them private static, you should be passing them around as parameters and return values, or take them in (from the constructor) as dependencies.

These private static variables make the code quite hard to follow, every method can access any of them, the message is not clear.

If you have ReSharper, it will suggest to make a method static if it's not accessing any instance members, or if it's only accessing static members. Don't take this suggestion as a recommendation - it's not. R# is only letting you know that the method can be made static. Nothing more. In general, it's best to steer away from anything static, especially when you're dealing with IDisposable stuff.

Some potential issues

Having private static object variables is one thing; having private static object variables that implement IDisposable is another.

The class is not thread-safe, and if you tried using multiple instances of it, you could potentially experience behavior I'm sure wasn't intended. Keep things well encapsulated at instance level. static puts a member at type level, which means everything static is shared between instances (hence static is Shared in VB).

Readers, writers, streams - all these things are disposable. Sure, you Close() them, but anything IDisposable should be .Dispose()'d.

Also:

do
    {
        int dataBytes = clientStream.Read(fileData, byteTotal, (fileData.Length - byteTotal));
        byteTotal += dataBytes;
    } while (byteTotal < fileData.Length || clientStream.DataAvailable);

This do loop will always iterate at least once. Thus, if the while condition is met from the start, I'm expecting this method to blow up. This would solve it:

while (byteTotal < fileData.Length || clientStream.DataAvailable)
{
    int dataBytes = clientStream.Read(fileData, byteTotal, (fileData.Length - byteTotal));
    byteTotal += dataBytes;
};

#region

Remove #region. Instead, consistently structure your classes - private readonly fields, constructors, properties and backing fields, public methods, private methods:

public class Foo
{
    private readonly Bar _dependency;

    public Foo(Bar dependency)
    {
        _dependency = dependency;
    }

    public Something FooBar { get; set; }

    private Something _barFoo;
    public Something BarFoo
    {
        get { return _barFoo; }
        set { _barFoo = value; }
    }

    public void DoSomething()
    {
    }

    private void DoSomethingElse()
    {
    }
}

Comments

That said, your usage of comments is almost impeccable: the only thing I'd say is, be careful to avoid writing a comment that can eventually turn into a lie:

/// 
/// Default Constructor Initializes a new list of files
/// and a new byte array set to the max chunk size
/// 

This one is saying what the implementation of the default constructor is doing. If you decide to ever inline the variables' initialization with their declaration (private IList files = new List();), you have a comment to maintain - better avoid that.

Another tiny little thing, is that comments should be properly punctuated - they're all missing a final dot

StackExchange Code Review Q#44921, answer score: 5