Downloading files from a Mozilla file server via FTP

All classes on GitHub repo

This is the code for downloading files from a FTP-server running Mozilla Fileserver.

FtpItem is the interface for the two classes FtpFile and FtpFolder; FtpFile represents a file on the server and FtpFolder represents a Folder directory on the server.

FtpUser might be better named FtpHandler or something, it was made to avoid having global variables in the WinForms UI class together with non-UI specific logic.

TransferProgress is a class which is passed from FtpItem and FtpFile through events to update the progress of a download or tell the UI when a directory is downloaded and ready to be shown in the listbox.

FtpFileList is the name of the listbox in which FtpItems are shown to the user. Debuglist currently prints information about what's happening behind the scenes to avoid stepping through to check non-critical logic.

Right now it seems like only one task can be run in the background at any time, i.e it's not possible to change to a directory that's not currently downloaded while a file is transferring; I'm currently working on fixing this.

FtpItem Class

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace FtpLibrary
{
public interface FtpItem
{

    string Name { get; }
    string Path { get; }
    int FileSize { get; }
    SortedList List { get; }
    void Download(string s, string t, string p);
    FtpItem PreviousItem { get; }
    int Count { get; }
    event Action DownloadProgress;

}
}

FtpFolder Class

```
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Security.Cryptography.X509Certificates;
using System.IO;
using System.Diagnostics;

namespace FtpLibrary
{
public class FtpFolder : FtpItem
{
private static int _folderAmount = 0;
private string _folderName;
private string _folderPath;
private FtpItem _parent;
private SortedList _folderList;
pub

public interface FtpItem

By convention, interface names in .net start with a capital i, so the expected name for that type would be IFtpItem.

The interface has quite a few members, which points toward too many responsibilities.

In particular, the SortedList List { get; } member smells.

...and the implementation confirms the doubt:

public SortedList List { get { return null; } }

The interface is trying to be too many things at once.

I'll leave the rest of the FTP code to other reviewers, and focus more on the client WinForms code.

namespace WindowsFormsApplication2

Really? This could have been WinFormsFtpClient or something... But moving on...

public Form1()
{
    InitializeComponent();

    ftpfileList.MouseDoubleClick += FtpfileList_MouseDoubleClick;
    SavePath.Text = $"C:/Users/{Environment.UserName}/Ftp";

    if (!Directory.Exists($"C:/Users/{Environment.UserName}/Ftp"))
    {
        Directory.CreateDirectory($"C:/Users/{Environment.UserName}/Ftp");
    }

}

This Form1 class should be the application's MainWindow or MainForm, or FtpClientForm ...pretty much anything but Form1.

But enough nitpicking over naming. That constructor is doing too much work. You may not realize, but the Visual Studio designer is calling that code - it's by running InitializeComponent that the designer knows what controls to instantiate, where to put them, how they're named, and everything else the designer knows about the controls on that form, it knows by running that InitializeComponent method, which is generated code that you can find in the other "half" of the partial class.

By creating a directory in the form's constructor, you're not only executing I/O code in a constructor - you're running I/O code at design time, in the WinForms designer. This is what your constructor should look like:

public Form1()
{
    InitializeComponent();
    ftpfileList.MouseDoubleClick += FtpfileList_MouseDoubleClick;
}

Everything else doesn't belong there. Actually, I/O doesn't belong in the UI at all.

There's a name for the pattern going on here: Smart UI - great for prototyping, not-so-great for scalability and testability. The FTP library doesn't contain application logic: your form does. You could extract the application logic into its own class, and implement some Model-View-Presenter pattern, leaving the "view" (/form) with nothing but presentation concerns to care about.

These handlers were presumably created by the designer by double-clicking the controls. Remove the handler assignation in InitializeComponent, and then remove this dead no-op code:

private void CancelDownload_Click(object sender, EventArgs e)
{

}

private void ProgressLabel_Click(object sender, EventArgs e)
{

}

I think the methods in the TransferProgress class could be implemented as properties/getters, if you ensured that none of the getters would ever throw a division by zero exception (because getters should never throw). It's not clear whether the PercentDone getter is immune from that exception, either.

The TransferSpeed method is working too hard, performing the same division at least twice:

public string TransferSpeed()
{
    if ((TotalTransfered / EllapsedTime) <= 1000)
    {
        return $"{TotalTransfered / EllapsedTime} kb/s ";
    }
    else
    {
        return $"{ (TotalTransfered / EllapsedTime) / 1000} + Mb/s ";
    }
}

Do it once, and reuse the result in the string interpolations.

Ellapsed is actually spelled Elapsed, but that isn't the worst problem with the naming of int _ellapsedTime - is that seconds? milliseconds? minutes? hours? years? who knows... The framework has a type dedicated to represent a time interval: System.TimeSpan. Use it!

Same goes for totalTransfered: it's not clear what the unit of measure is.

StackExchange Code Review Q#114116, answer score: 7