File transfer over a stream

Question

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<string> files;
        private static NetworkStream clientStream;
        private static byte[] fileData;
        private static UInt32 dataLength;
    #endregion

    #region Functions

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

    /// <summary>
    /// Uses the Given File Stream to receive a file
    /// </summary>
    /// <returns>Returns a bool depending on if the file was succesfully received or not.</returns>
    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;
    }

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

    /// <summary>
    /// Gets the next chunk of data from the file
    /// </summary>
    /// <returns> Returns the total number of bytes read this chunk</returns>
    private static int GetNextChunk()
    {
        int byteTotal = 0;
        do
        {
            int dataBytes = clientStream.Read(fileData, byteTotal, (fileData.Length - byteTotal));
            byteTotal += dataBytes;
        } while (byteTotal < fileData.Length || clientStream.DataAvailable);
        return byteTotal;
    }

    /// <summary>
    /// Opens the file path to save the file in
    /// Checks for existence of file and renames file with (number) if it does exist
    /// </summary>
    /// <param name="filePath">file path to save file to</param>
    private static void OpenSaveFilePath(string filePath)
    {
        int enumeration = 1;
        string tempPath = filePath.Substring(0, filePath.IndexOf(".", StringComparison.Ordinal));
        string tempExtension = filePath.Substring(filePath.IndexOf(".", StringComparison.Ordinal), (filePath.Length - tempPath.Length));
        if (File.Exists(filePath))
        {
            while (File.Exists(tempPath + tempExtension))
            {
                tempPath = filePath.Substring(0, filePath.IndexOf(".", StringComparison.Ordinal));
                tempPath += "(" + enumeration + ")";
                enumeration++;
            }
            filePath = tempPath + tempExtension;
        }
        fileWriter = new BinaryWriter(File.Open(filePath, FileMode.CreateNew));
    }

    /// <summary>
    /// Gets the file name from the current client stream
    /// </summary>
    /// <returns>Returns the file name as a string</returns>
    private static string GetFileName()
    {
        byte[] fileNameLengthBytes = new byte[sizeof(int)];
        clientStream.Read(fileNameLengthBytes, 0, 4);
        int fileNameLength = BitConverter.ToInt32(fileNameLengthBytes, 0);

        byte[] fileName = new byte[fileNameLength];
        clientStream.Read(fileName, 0, fileNameLength);

        return Encoding.UTF8.GetString(fileName, 0, fileNameLength);
    }

    /// <summary>
    /// Get the length of the file from the received client stream
    /// </summary>
    /// <returns>Returns the length of the file</returns>
    private static UInt32 GetFileLength()
    {
        byte[] length = new byte[sizeof(UInt32)];
        clientStream.Read(length, 0, sizeof(UInt32));
        return BitConverter.ToUInt32(length, 0);
    }

    /// <summary>
    /// Sets the size of the byte array depending on number of bytes left to send/receive
    /// </summary>
    /// <param name="file">byte array to set</param>
    /// <param name="bytesLeft">number of bytes left to send/receive</param>
    private static void SetByteArray(out byte[] file, UInt32 bytesLeft)
    {
        file = bytesLeft < MAX_CHUNK_SIZE ? new byte[bytesLeft] : new byte[MAX_CHUNK_SIZE];
    }

    /// <summary>
    /// Retrieves all the files from the Files folder in the current directory
    /// </summary>
    private static void GetCurrentFiles()
    {
        DirectoryInfo directory = new DirectoryInfo("Files");
        files = directory.GetFiles().OfType<string>().ToList<string>();
    }


    /// <summary>
    /// Sends the preliminary data to the client 
    /// file name and length of file in bytes
    /// </summary>
    /// <param name="filePath">File path to file</param>
    private static void SendFileData(string filePath)
    {
        string fileName = Path.GetFileName(filePath);

        fileReader = new FileStream(filePath, FileMode.Open, FileAccess.Read);
        UInt32 numberOfBytesToRead = (UInt32)fileReader.Length;

        if (fileName != null)
        {
            byte[] fileNameBytes = Encoding.UTF8.GetBytes(fileName);
            byte[] fileNameLength = BitConverter.GetBytes(fileName.Length);
            byte[] dataSize = BitConverter.GetBytes(numberOfBytesToRead);
            byte[] clientData = new byte[4 + fileName.Length + sizeof(UInt32)];

            fileNameLength.CopyTo(clientData, 0);
            fileNameBytes.CopyTo(clientData, 4);
            dataSize.CopyTo(clientData, (fileName.Length + 4));

            clientStream.Write(clientData, 0, clientData.Length);
        }
    }

    /// <summary>
    /// Sends a file to the remote client
    /// </summary>
    /// <param name="client">Client to send file to</param>
    /// <param name="filePath">File Path to the file</param>
    /// <returns></returns>
    public bool SendFile(TcpClient client, string filePath)
    {
        clientStream = client.GetStream();
        SendFileData(filePath);
        UInt32 fileSizeInBytes = (UInt32)fileReader.Length;
        int numberOfBytesRead = 0;

        SetByteArray(out fileData, fileSizeInBytes);

        while (numberOfBytesRead < fileSizeInBytes)
        {
            numberOfBytesRead += WriteFileToStream(fileSizeInBytes);
        }
        fileReader.Close();
        return true;
    }

    /// <summary>
    /// Reads the bytes from the file and sends it over the network
    /// </summary>
    /// <param name="fileSizeInBytes">Total size of file in bytes</param>
    /// <returns>returns number of bytes read from the file</returns>
    public static int WriteFileToStream(UInt32 fileSizeInBytes)
    {
        int done = fileReader.Read(fileData, 0, fileData.Length);
        clientStream.Write(fileData, 0, fileData.Length);
        SetByteArray(out fileData, fileSizeInBytes);
        return done;
    }
    #endregion
    #region Accessors
    public int GetMaxChunkSize
    {
        get { return MAX_CHUNK_SIZE; }
    }

    public BinaryWriter FileWriter
    {
        get { return fileWriter; }
    }

    public FileStream FileReader
    {
        get { return fileReader; }
    }

    public List<string> GetFiles
    {
        get { return files; }
    }

    public NetworkStream ClientStream
    {
        set { clientStream = value; }
        get { return clientStream; }
    }

    public UInt32 GetDataLength
    {
        get { return dataLength; }
    }
    #endregion
}

Answer 1

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:

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

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<string> files = new List<string>();), 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