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.Proxy = null;

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

while (!string.IsNullOrEmpty(line))
{
}

using (var ftpClient = new WebClient())
{

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.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); } } }  ## 1 Answer 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())
{
{
}
}
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.

• Appreciated, will go through this once out of the office! And yes, string interpolation is amazing for easy to read syntax! Worth looking into! Jul 21 '15 at 9:32