Downloading all files in a FTP folder and then deleting them

Question

I've created two methods in a class that allow me to download the contents of an FTP folder and if specified then delete them. Although the code operates as intended I think it's vastly bloated and I'm looking for direction on how to correct it.

Of note I believe that there's far too many FTP connections opened in a single file move (three I believe - list directory, download files, delete files). I'm a bit unsure how to refine this.

public void DownloadFolder(string localFilesPath, string remoteFTPPath, bool deleteAfterDownload = false)
{
    remoteFTPPath = "ftp://" + Hostname + remoteFTPPath;
    var request = (FtpWebRequest)WebRequest.Create(remoteFTPPath);

    request.Method = WebRequestMethods.Ftp.ListDirectory;
    request.Credentials = new NetworkCredential(Username, Password);
    request.Proxy = null;

    FtpWebResponse response = (FtpWebResponse)request.GetResponse();
    Stream responseStream = response.GetResponseStream();
    StreamReader reader = new StreamReader(responseStream);
    List<string> directories = new List<string>();

    var line = reader.ReadLine();

    while (!string.IsNullOrEmpty(line))
    {
        directories.Add(line);
        line = reader.ReadLine();
    }
    reader.Close();

    using (var ftpClient = new WebClient())
    {
        ftpClient.Credentials = new NetworkCredential(Username, Password);

        for (var i = 0; i <= directories.Count - 1; i++)
        {
            if (!directories[i].Contains("."))
            {
                continue;
            }

            var path = remoteFTPPath + "/" + directories[i].Remove(0, directories[i].IndexOf(@"/") + 1);
            var transferPath = localFilesPath + @"\" + directories[i].Replace(@"/", @"\");

            PostEvent($"Attempting to download File: {path} to: {transferPath}", Debug);

            try
            {
                ftpClient.DownloadFile(path, transferPath);
                PostEvent($"Downloaded File: {path} to: {transferPath}", Info);

                PostEvent($"Preparing to delete file: {path}\n\n", Debug);

                if (deleteAfterDownload)
                {
                    DeleteFile(path);
                }
            }
            catch (WebException ex)
            {
                PostEvent($"Error downloading or deleting file {path} to {transferPath}", Error);
                PostEvent($"Exception: {ex.Message}", Error);
            }
            catch (Exception ex)
            {
                PostEvent($"General Exception: {ex.Message}", Error);
            }
        }
    }

    response.Close();
}

public void DeleteFile(string remoteFTPPath)
{
    FtpWebRequest request = (FtpWebRequest)WebRequest.Create(remoteFTPPath);

    request.Method = WebRequestMethods.Ftp.DeleteFile;
    request.Credentials = new NetworkCredential(Username, Password);
    request.Proxy = null;

    FtpWebResponse response = (FtpWebResponse)request.GetResponse();

    PostEvent($"Deleting file {remoteFTPPath} returned status {response.StatusDescription}", Debug);

    response.Close();
}

Here's the class initializer (where my credentials are passed into properties within this class):

public FtpHelper(string ftpHostname, string ftpUsername, string ftpPassword)
{
    Hostname = ftpHostname;
    Username = ftpUsername;
    Password = ftpPassword;
}

And the postevent method that's referred to here come's from a base class I inherit to this class (unsure if this is bad practice, please let me know if so):

public class BaseHelper
{
    private EventHandler<BaseExceptionEventArgs> _onEvent;

    public event EventHandler<BaseExceptionEventArgs> OnEventHandler
    {
        add { _onEvent += value; }
        remove { _onEvent += value; }
    }

    /// <exception cref="Exception">A delegate callback throws an exception.</exception>
    public void PostEvent(string message, BaseExceptionEventArgs.ExceptionLevel exceptionLevel,
        Exception exception = null)
    {
        if (_onEvent == null) return;

        if (exception == null)
        {
            var e = new BaseExceptionEventArgs(message, exceptionLevel);
            _onEvent(this, e);
        }
        else
        {
            var e = new BaseExceptionEventArgs(message, exceptionLevel, exception);
            _onEvent(this, e);
        }
    }
}

Answer 1

The first thing I would do is change your constructor:

private readonly NetworkCredential credentials;

public ftpHelper(string ftpHostname, string ftpUsername, string ftpPassword)
{
    credentials = new NetworkCredential(ftpUsername, ftpPassword);
    Hostname = ftpHostname;
}

That eliminates newing up credentials all over your code.

As for the rest of your code, I think a few well named functions will make the code much more readable. For example, getting a list of files:

private IEnumerable<string> ListDirectories(string remoteFtpPath)
{
    var url = $"ftp://{Hostname}{remoteFtpPath}"; // Is this right? Not used C#6 yet...

    var request = (FtpWebRequest)WebRequest.Create(url);
    request.Method = WebRequestMethods.Ftp.ListDirectory;
    request.Credentials = credentials;
    request.Proxy = null;
    
    var directories = new List<string>();
     
    using (var response = (FtpWebResponse)request.GetResponse())
    using (var responseStream = response.GetResponseStream())
    using (var reader = new StreamReader(responseStream))
    {
        while (!reader.EndOfStream)
        {
            directories.Add(line); 
        }
    }
    return directories;
}

You could write the above using yield if you'd rather.

Go through your code and keep pulling out all of the methods aiming to get each one to do a single thing.

For example, I'd probably pull out another method from the above (ListDirectories) called CreateListDirectoriesRequest and have that create the WebRequest.

Other thoughts

Ftp or ftp never FTP in names.

What you're doing with your remoteFTPPath parameter is very confusing. I'd have no idea what it was for a lot of your method.

It's easier to use using than calling Close/Dispose most of the time.

You almost certainly want to log ex.ToString() rather than just the message. You'll want the stack trace when you're tracking down problems.