3
\$\begingroup\$

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;

            return _result;
        }
        finally
        {
            requestFileDownload = null;
            responseFileDownload = null;

        }


    }

    public struct Result
    {
        public bool Error;
        public string ErrorMessage;
        public string ErrorFileName;
        public string ErrorFilePath;
        public List<string> Files;
    }
\$\endgroup\$
4
\$\begingroup\$

A few things:

  1. 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?

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

  3. 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?

  4. 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.

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

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

  7. 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.

\$\endgroup\$
  • \$\begingroup\$ first thx so much for your answer , yes its static a sample , i will re-write hardcoded varibles later.I edited code a bit.My concern is i run this download function maybe 20 times(count of files in ftp folder).Must i use an ftp.dll for this job ? so i tried an ftp.dll for .net , this dll open connection once and gives to you dodownload() , doupload() , dorename() etc. functions.But somethimes i got socket error , missing file downloading etc. So i want to trust Framework libruary so i want to write myself :) \$\endgroup\$ – Mennan Oct 13 '12 at 21:14
  • \$\begingroup\$ I wouldn't suggest accepting this answer just yet Mennan. I'm pretty sure there are still lots of people that could contribute more or better to your question. Perhaps you might want to let it sit for a few days and see what other comments you get? \$\endgroup\$ – dreza Oct 13 '12 at 22:41
  • \$\begingroup\$ ok thx dreza... \$\endgroup\$ – Mennan Oct 13 '12 at 23:06
  • 1
    \$\begingroup\$ You might want to consider removing the _ from your variable names. See msdn.microsoft.com/en-us/library/xzf533w0%28v=vs.71%29.aspx. also, you can remove the first bytesRead = line as that is covered in the while loop. \$\endgroup\$ – dreza Oct 13 '12 at 23:32
4
\$\begingroup\$

If this process executes more than a single command in each interval, such as: upload something, then rename a file, then download something else - then it's not optimized.

I suggest you use some kind of a library for FTP session, in which you should execute all of the commands in a single session. This should be better in terms of performance, since you're already connected to the FTP server, and therefore you already passed the firewalls and the FTP authentication.

Update: so far I used several FTP libraries, some were free (long time ago, around .NET 1.1), some not (recently). I can't recommend a free library since I haven't recently used one. I suggest visiting free ftp library question (and answers) in stackoverflow for a discussion about the subject. However, I recently used the non free Rebex FTP library, and it's excellent.

\$\endgroup\$
  • 2
    \$\begingroup\$ what you suggest as opensource and stable ftp libruary ? \$\endgroup\$ – Mennan Oct 14 '12 at 16:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.