Downloading all files in a FTP folder and then deleting them

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 directories = new List();

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);

Pos

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 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();
     
    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.

StackExchange Code Review Q#97534, answer score: 5