4
\$\begingroup\$

I've written a method to check if a directory already exists on an FTP server. The only way I could see to make this work was to return a different result from the method if an exception is thrown - very bad practice in my opinion. Is there a better way to write this?

public bool DirectoryExists(string directory)
{
    if (String.IsNullOrEmpty(directory))
        throw new ArgumentException("No directory was specified to check for");

    // Ensure directory is ended with / to avoid false positives
    if (!directory.EndsWith("/"))
        directory += "/";

    try
    {
        var request = (FtpWebRequest)WebRequest.Create(directory);
        request.Method = WebRequestMethods.Ftp.ListDirectory;
        request.Credentials = new NetworkCredential(Username, Password);

        using (request.GetResponse())
        {
            return true;
        }
    }
    catch (WebException)
    {
        return false;
    }
}

The connection hostname, username and password referred to within this block of code are auto properties set when the class is initialized.

\$\endgroup\$
1
\$\begingroup\$
  • don't omit braces {} for single if statements. Using braces in this case will make your code less errorprone.

  • you want to return a state, which represents a specific states which shouldn't be restricted to only two states. This just calls for an enum.

    public enum FtpResponse
    {
        DirectoryExists, DirectoryNotFound, DirectoryNotSpecified
    }
    
    
    public FtpResponse DirectoryExists(string directory)  
    {
        if (String.IsNullOrEmpty(directory))
        {
             return FtpResponse.DirectoryNotSpecified;
        } 
        // the rest of the code
    
    }
    
\$\endgroup\$
1
  • 6
    \$\begingroup\$ I think it's better to throw an argument exception if the argument is invalid rather that returning another enum value. The single line if suggestion isn't helpful either, it's just one of the useless programmer holy wars. \$\endgroup\$ Mar 3 '15 at 13:44
1
\$\begingroup\$

(Missing) Specification

You are lacking a clear specification (comments). The way you implemented it, you consider not exists and don't know whether it exists as one and the same. I would expect a method called DirectoryExists to:

  • return true when the directory exists
  • return false when it does not exist
  • throw an exception when unabe to determine existence

Let consumers handle exceptions:

catch (WebException)
{
    //return false;
    throw;                  // if we don't log here, remove the catch altogether
}

It may very well be possible you cannot find a way to determine when to return false. In such case, the method either returns true or throws an exception. I would allow such behavior to be forward-compatible in case we ever do find a way to return false without impacting legacy consumer code.

If the purpose of this method is not to swallow exceptions (as your question title suggests), and only to return true or false, you should definately put in the specification that the method is a sand-box.

Valid paths

Isn't it possible that an empty path means the current directory?

if (String.IsNullOrEmpty(directory))
    throw new ArgumentException("No directory was specified to check for");

Not sure whether edge cases could escape your code below. But I would favour dispatching as much as we can to built-in functionality.

// Ensure directory is ended with / to avoid false positives
if (!directory.EndsWith("/"))
    directory += "/";

GetFullPath ensures at most one folder separator are appended. So I've turned the condition around. Always add the separator, and let GetFullPath clean it up.

directory = Path.GetFullPath(directory + "/");
\$\endgroup\$
0

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.