Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'd like to check the following process relating to the uploading of data by FTP, after the data is read from a database.

I have a stored procedure that returns data in the following pipe delimited format in a single column. It typically returns around 300 rows, each row has about 300 bytes.

1234|||||PROPERTY|||MARKET INDEX|||||ADDRESS|||||1||||GBP|||||||20110930||||||||C|OFFICE|F||

The application then submits that data by FTP, typically <100k overall.

Here's the mechanism used, restricted to .NET 3.5.

// database method returns dataset/datareader, converted and passed to method as
// IEnumerable<string> data

string ftpServerIP = "5.4.3.2:21";
string targetFileName = "mann.txt";
string username = "manfred";
string password = "*******";

Uri uri = new Uri(String.Format("ftp://{0}/{1}", ftpServerIP, targetFileName));    
FtpWebRequest reqFTP = (FtpWebRequest)FtpWebRequest.Create(uri);
reqFTP.Credentials = new NetworkCredential(username, password);
reqFTP.Method = WebRequestMethods.Ftp.UploadFile;
reqFTP.KeepAlive = false;
reqFTP.UsePassive = false;

MemoryStream stIn = new MemoryStream();
using (StreamWriter sw = new StreamWriter(stIn))
{
    foreach (string item in data)
    {
        sw.WriteLine(item);
    }

    sw.Flush();

    using (Stream stOut = reqFTP.GetRequestStream())
    {
        stOut.Write(stIn.GetBuffer(), 0, (int)stIn.Length);
    }
}

FtpWebResponse response = (FtpWebResponse)reqFTP.GetResponse();
response.Close();

I'd like to check whether the stream and ftp processing looks ok, or whether there are other stream types to consider (BufferedStream?), or closure/disposal steps, etc...

share|improve this question
    
Looks like you are not dealing with a lot of data, so speed is not a huge concern, thus you could separate code (FTP from DB) a bit. I personally would have a method that returns a LinkedList<string> or IEnumerable<string> of stuff that comes from the database. Then I would pass that object to the method with the FTP logic. If your code in this project will never be larger than two pages, then this probably does not matter. My other small pieves are: I would skip a line after } (as StyleCop would suggest). I would also build out a URI with String.Format. –  Leonid Feb 10 '12 at 16:24
    
@Leonid, apologies for the extremely tardy reply. If you haven't since retired ... and want to add these notes as an answer, I'll add my vote. Question updated with your suggestions meanwhile (though I need to check the StyleCop implementation that you mentioned). –  richaux Sep 29 '13 at 16:37
add comment

1 Answer 1

up vote 2 down vote accepted

In general it looks fine to me. BufferedStream would not help anything here, your data is already in memory by the time any streams get created.

I would prefer to see the MemoryStream in a using block, although in this case it is being disposed when the StreamWriter is disposed.

It may be a good idea to check the response and handle any error status. If you do ever add any code that uses the FtpWebResponse, I would change to a using block, instead of the call to close.

Not really related to the code, but avoid using FTP if you can.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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