FTP Download Helper Method

Question

I'm currently using an Open Source FTP library in all my projects as I've never managed to code a decent FTP Helper Library based on the C# 4.0 API.

What do you think about this approach? I'm not quite versed in TDD but ideally I'd like to make this class as testable as possible.

public void DownloadFile(string host, string file, string user, string pass, string path)
{
 if (!String.IsNullOrWhiteSpace(host) && !String.IsNullOrWhiteSpace(file))
 {
 Uri addr = new Uri(host + file);
 using (WebClient client = new WebClient())
 {
 if (!String.IsNullOrWhiteSpace(user) && !String.IsNullOrWhiteSpace(pass))
 {
 client.Credentials = new NetworkCredential(user, pass);
 }
 if (!String.IsNullOrWhiteSpace(path))
 {
 client.DownloadFile(addr, path);
 }
 }
 }
}

Answer 1

Here's how I would do it, with comments!

public void DownloadFile(string host, string file, string user, string pass, string path)
{
 // Inverted if-statement to reduce nesting
 if (String.IsNullOrWhiteSpace(host) 
 throw new ArgumentNullException("host");
 if(String.IsNullOrWhiteSpace(file))
 throw new ArgumentNullException("file");
 //Moved path check to forefront because nothing would happen if it's empty
 if(String.IsNullOrWhiteSpace(path)) 
 throw new ArgumentNullException("path");
 // Changed to UriBuilder. This will handle cases like trailing slashes and whatnot
 UriBuilder address = new UriBuilder();
 address.Host = host;
 address.Path = file;
 using (WebClient client = new WebClient())
 {
 if (!String.IsNullOrWhiteSpace(user) && !String.IsNullOrWhiteSpace(pass))
 {
 client.Credentials = new NetworkCredential(user, pass);
 }
 // Changed to a try catch so that we can handle the error with a better message
 // If you'd rather have this bubble up, that's fine
 try 
 {
 client.DownloadFile(address.Uri, path);
 }
 catch (Exception e)
 {
 throw new InvalidOperationException(
 "Error downloading file from: " + address.Uri.ToString(), 
 e)
 }
 }
}

I would also change the variable names as Snowbear HIM-compiler said.

I like to keep guard conditions at the top where the cheapest is first. Also, providing exceptions makes it easier for the caller to know why something failed. If you want to switch it to a boolean success/fail, that could work as well, but doesn't provide as much information.

Answer 2

Unit testing means that automated tests can run in isolation from any dependencies (such as other classes, but also the environment).

The DownloadFile method has a hard-coded dependency on WebClient and thus on the environment. It can't be used without using the network and (presumably) a remote service. Even if you decide to hit localhost, you'll need to set up an FTP server on your own machine, and that counts as an external dependency as well. Thus the method can't be unit tested as is.

One possible remedy is to hide WebClient behind an appropriate interface (you could call it IWebClient) and inject that into the class holding the DownloadFile method.

In general, however, such methods are better left as Humble Objects and perhaps subjected to integration tests.

• Brian Reichle: I agree on all but the idea of IWebClient. If I was going to use dependency injection for testing, I personally would prefer to use a RealProxy based mocking system rather than create an interface & wrapper class (since WebClient already inherits from MarshalByrefObject).

Answer 3

1) Your method has no way to tell whether it has downloaded a file or not. You validate input parameters but do not throw exceptions if they are empty which is wrong. Otherwise you simply postpone receiving errors.

2) new Uri(host + file); I don't think concatenation is correct operation for Uri parts, I would look for something like Path.Combine for this purpose. As far as I got Uri(Uri baseUri, string relativeUri) constructor will do the job.

3) If user and pass parameters are optional (you're checking them to be not null) then define them as optional. It will be more clear for users who will use such method that they can omit user and pass parameters if they see that it is optional. Otherwise they will have to guess whether they can pass null or not.

4) There is no point validating path at the end of the method. Anyway the method will not do anything if path is empty. Validate it at the beginning together with host and file.

5) I think you should rename your parameters. You have path together with file and it is not that easy to guess which one is local path and which is ftp path. Having path at the end makes it a little bit easier to guess, but still not enough. Name them remotePath or ftpPath and localPath correspondingly.