Is this FTP download code useful?

I have a Windows-service and this service will connect to an ftp server and do download, upload, and rename operations.

My service runs every 30 seconds, so the ftp server will be busy during these operations.

My question is: is my code useful for this job? Because my download/upload/rename functions open a webrequest each process? I use FtpWebRequest and FtpWebResponse

I write only the download functions, the other functions are similar.

Download function:

```
public Result Download(string localPath,string remotePath,string fileName)
{
Result _result = new Result();
FtpWebRequest requestFileDownload = null;
FtpWebResponse responseFileDownload = null;

try
{

//
requestFileDownload =
(FtpWebRequest)WebRequest.Create(remotePath + fileName);
//
requestFileDownload.Credentials =
new NetworkCredential(this.Username, this.Password);
//
requestFileDownload.Method =
WebRequestMethods.Ftp.DownloadFile;

responseFileDownload =
(FtpWebResponse)requestFileDownload.GetResponse();

using (Stream responseStream = responseFileDownload.GetResponseStream())
{
FileStream writeStream = new FileStream(localPath + fileName, FileMode.Create);

int Length = 2048;
Byte[] buffer = new Byte[Length];

int bytesRead;
while((bytesRead = responseStream.Read(buffer, 0, Length)) > 0)
{
writeStream.Write(buffer, 0, bytesRead);
}

writeStream.Close();

}

return _result;

}
catch (Exception x)
{
_result.Error = true;
_result.ErrorMessage = x.StackTrace;
_result.ErrorFileName = fileName;
_result.ErrorFilePath = remotePath + fileName;

A few things:

-
I would supply all the variables that you have hard coded into the method i.e. localPath, filename, ftp server, networkcredentials. Otherwise how can you re-use this method for other operations?

-
I would probably avoid making the method static unless there was good reason to.

-
In your finally clause you don't check if responseStream or writeStream are null? What if they never got created before an exception was thrown?

-
I personally think unless there is a very valid reason you should always do something in an exception catch block, whether it be logging somewhere, re-throwing the exception or anything. Otherwise the calling method will have no idea that something went wrong.

-
What is _result? I don't see it being set and I don't see it being declared anywhere?

-
If Length is a constant, make it a constant. i.e. const int Length = 2048.

-
I might also consider implementing some sort of call back method so you can provide status updates on the progress on the download. Just a suggestion, not sure how you would implement it exactly.

I might consider writing the loop as such:

int bytesRead;
while((bytesRead = responseStream.Read(buffer, 0, Length)) > 0)
{
   writeStream.Write(buffer, 0, bytesRead);
}

As to your quesion. Is this code useful? In short. Yes, but only if you ever want to do only one thing with it and never want to re-use anything inside. Otherwise no.

That's just a few comments off the top of my head. I hope those are a good starting point.

StackExchange Code Review Q#17506, answer score: 4