7
\$\begingroup\$

I've written a FTPHelper class which should help to teach me more about code structure.
I don't know what I don't know so I would really value feedback on how I can be laying out and thinking about my programming better!

public class FtpHelper : BaseHelper
{
    public FtpHelper(string ftpHostname, string ftpUsername, string ftpPassword)
    {
        Hostname = ftpHostname;
        Username = ftpUsername;
        Password = ftpPassword;
    }

    private string Hostname { get; set; }
    private string Username { get; set; }
    private string Password { get; set; }

    public void UploadFilesinFolder(string sourcePath, string destinationPath, string fileType = "*.*")
    {
        PostEvent("Destination Path: " + destinationPath, FtpEventArgs.ExceptionLevel.Debug);

        if (String.IsNullOrEmpty(destinationPath)) throw new Exception("No files in destination folder or desintation folder was not specified");

        foreach (var file in Directory.GetFiles(sourcePath, fileType))
        {
            UploadFile(file, destinationPath);
        }
    }

    /// <summary>
    /// Check if a directory exists.
    /// </summary>
    /// <param name="directory"></param>
    /// <returns></returns>
    public bool DirectoryExists(string directory)
    {
        // todo: Check if directory var has a trailing '/', if not, add it. Otherwise false positives are thrown

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

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

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

    public FtpStatusCode CreateDirectory(string destination)
    {
        var folderRequest = WebRequest.Create(destination);
        folderRequest.Credentials = new NetworkCredential(Username, Password);
        folderRequest.Method = WebRequestMethods.Ftp.MakeDirectory;

        try
        {
            using (var resp = (FtpWebResponse)folderRequest.GetResponse())
            {
                return resp.StatusCode;
            }
        }
        catch (Exception ex)
        {   
            throw new Exception("Unable to create directory " + destination + " Details:" + ex.Message);
        }
    }

    private FtpStatusCode ProcessFile(string source, string destination)
    {
        if (String.IsNullOrEmpty(source))
            throw new Exception("No source specified, cannot process file");

        if(String.IsNullOrEmpty(destination))
            throw new Exception("No destionation specified, cannot process source: " + source);

        var sourceFile = WebHelper.AppendPaths(destination, Path.GetFileName(source));

        PostEvent("Attempting to upload: " + sourceFile, BaseExceptionEventArgs.ExceptionLevel.Debug);

        var request = WebRequest.Create(sourceFile);

        request.Credentials = new NetworkCredential(Username, Password);
        request.Method = WebRequestMethods.Ftp.UploadFile;

        // todo: split create directory and upload file into two areas

        using (var resp = (FtpWebResponse)request.GetResponse())
        {
            return resp.StatusCode;
        }
    }

    public void UploadFile(string sourceFile, string destinationPath)
    {
        if (String.IsNullOrEmpty(destinationPath))
            throw new Exception("Empty Destination Path");

        if(string.IsNullOrEmpty(sourceFile))
            throw new Exception("No source file specified");

        try
        {
            PostEvent("Destination Path: " + destinationPath, BaseExceptionEventArgs.ExceptionLevel.Debug);

            var destination = "ftp://" + Hostname + destinationPath;

            PostEvent("Checking if exists: " + destination, BaseExceptionEventArgs.ExceptionLevel.Debug);

            // check if destination directory exists and if not create it
            if (!DirectoryExists(destination))
            {
                PostEvent("Attempting to create directory: " + destination, BaseExceptionEventArgs.ExceptionLevel.Debug);

                var directoryStatus = CreateDirectory(destination);

                PostEvent("FTP Response: " + directoryStatus, BaseExceptionEventArgs.ExceptionLevel.Debug);
            }
            else
            { 
                PostEvent("Directory already exists: " + destination, BaseExceptionEventArgs.ExceptionLevel.Debug);
            }

            // upload file
            PostEvent("Attempting to upload " + sourceFile + " to " + destinationPath, BaseExceptionEventArgs.ExceptionLevel.Debug);

            var fileStatus = ProcessFile(sourceFile, destinationPath);

            PostEvent("FTP Response: " + fileStatus, BaseExceptionEventArgs.ExceptionLevel.Info);
        }
        catch (Exception ex)
        {
            PostEvent("Error when uploading file: " + sourceFile, BaseExceptionEventArgs.ExceptionLevel.Error, ex);
        }
    }
}

Base Helper Class

public class BaseHelper
{
    private EventHandler<BaseExceptionEventArgs> _onEvent;

    public event EventHandler<BaseExceptionEventArgs> OnEventHandler
    {
        add { _onEvent += value; }
        remove { _onEvent += value; }
    }

    public void PostEvent(string message, BaseExceptionEventArgs.ExceptionLevel exceptionLevel, Exception exception = null)
    {
        if (_onEvent == null) return;

        if (exception == null)
        {
            var e = new BaseExceptionEventArgs(message, exceptionLevel);
            _onEvent(this, e);
        }
        else
        {
            var e = new FtpEventArgs(message, exceptionLevel, exception);
            _onEvent(this, e);
        }
    }
}
\$\endgroup\$
0

2 Answers 2

6
\$\begingroup\$

I would put the guard clause first - before calling the PostEvent base class method:

if (String.IsNullOrEmpty(destinationPath)) throw new Exception("No files in destination folder or desintation folder was not specified");

Now that would involve less horizontal scrolling with proper bracing:

if (String.IsNullOrEmpty(destinationPath))
{
    throw new Exception("No files in destination folder or desintation folder was not specified");
}

And now the exception type and its message become more apparent.

  • Don't throw System.Exception - here an ArgumentException would be a much better fit. Always try to throw meaningful exception types.. create your own if you have to.
  • The cake message is a lie. I don't see how that method can have any clue whatsoever as to how many files are in the specified destination folder. If the caller is assumed to have verified that, then this method must not make such an assumption. Misleading exception messages can make code harder to debug than it needs to be.

Same thing here - I'll just add that in no-brace code, I prefer this to the above.. but consistency in style is always a better choice:

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

The message is better though, only missing punctuation.


    catch (Exception ex)
    {   
        throw new Exception("Unable to create directory " + destination + " Details:" + ex.Message);
    }

This is a very specific message, for the widest possible exception. By re-throwing (again, don't throw System.Exception) like this, you are also losing the stack trace information from the original exception - not good. A better practice would be to throw a custom exception type, and embedding the original exception as an InnerException.

\$\endgroup\$
2
  • \$\begingroup\$ Great feedback, thank you :) Anything you feel I should study to improve as a developer based on what you've seen in this code? \$\endgroup\$
    – Michael A
    Commented Dec 15, 2014 at 2:54
  • 1
    \$\begingroup\$ I'd say exception handling - learn what existing exceptions are readily available for you to use (and when to throw them), and when to create your own exception types. \$\endgroup\$ Commented Dec 15, 2014 at 2:55
5
\$\begingroup\$

Now, I'm not quite sure how to fix this, but I think I smell a smell here.

var e = new FtpEventArgs(message, exceptionLevel, exception);

The base class shouldn't know about any child classes. As you have it, any time you create a new class that inherits from BaseHelper you would have to go back and modify BaseHelper (potentially breaking it and any classes that inherit from it.) I would definitely spend some time thinking about how you could make it so that BaseHelper is "closed" and "finished" code that you never have to think about again. It looks like you're trying to implement the Visitor pattern, so you might want to take a look at some examples.

\$\endgroup\$
2
  • \$\begingroup\$ Great feedback, thanks. Will give this some thought. \$\endgroup\$
    – Michael A
    Commented Dec 15, 2014 at 3:57
  • 1
    \$\begingroup\$ Actually - looks like legacy code before inheritance of a base class was introduced. It hadn't caused a problem because FTPEventArgs was inheriting BaseExceptionEventArgs as well. I've now fixed this and removed the now redundant FTPEventArgs - thank-you! \$\endgroup\$
    – Michael A
    Commented Dec 15, 2014 at 4:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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