0
\$\begingroup\$

I was refactoring some of my utility code modules used in my test projects, and wanted to apply my knowledge of clean OOP design patterns and SOLID principles to make these modules more useful and easier to use.

I have an interface called IFtpFileCleaner:

/// <summary>
/// Responsible for deleting a single file located in a remote server
/// </summary>
public interface IFtpFileCleaner
{
    /// <summary>
    /// Deletes a remote file
    /// </summary>
    /// <param name="remotePath">
    /// The full remote path.
    /// </param>
    void Clean(string remotePath);
}

I have an abstract class called RemoteFileCleaner, which implements IFtpFileCleaner:

/// <summary>
/// The file cleaner base class provides common code for all the implementations to reduce code duplication
/// </summary>
public abstract class RemoteFileCleaner : IFtpFileCleaner
{
    /// <summary>
    /// The FTP _client.
    /// </summary>
    private readonly IFtpSecureClient _client;

    /// <summary>
    /// Initializes a new instance of the <see cref="RemoteFileCleaner"/> class.
    /// </summary>
    /// <param name="client">
    /// The FTP client.
    /// </param>
    protected RemoteFileCleaner(IFtpSecureClient client)
    {
        _client = client;
    }

    /// <summary>
    /// Deletes a single file located on a remote server
    /// </summary>
    /// <param name="remotePath">
    /// The full path of the remote file.
    /// </param>
    public void Clean(string remotePath)
    {
        try
        {
            _client.Init();
            _client.DeleteFile(remotePath);
        }
        catch (Exception e)
        {
            Console.WriteLine($"Failed to clean file {remotePath}.{Environment.NewLine}{e}");
            throw;
        }
        finally
        {
            _client.Disconnect();
        }
        Console.WriteLine($"Remote File {remotePath} was deleted.");
    }
}

Now, I can have concrete implementations of my file cleaners. Like SFTPFileCleaner, FTPFileCleaner, and FTPSFileCleaner. They all look very similar. For instance, SFTPFileCleaner looks like this:

/// <summary>
/// The SFTP implementation of <see cref="RemoteFileCleaner"/>.
/// </summary>
public class SFTPFileCleaner : RemoteFileCleaner
{
    /// <summary>
    /// Initializes a new instance of the <see cref="SFTPFileCleaner"/> class.
    /// </summary>
    public SFTPFileCleaner()
        : base(new FtpSecureClientTestFactory().CreateSFTPClient())
    {
    }
}

As you can see, I am performing "Bastard Injection" here in the concrete implementation because I would like to easily instantiate these cleaners in my test methods. Here is an example:

[TearDown]
public void Cleanup()
{
    _ftpCleaner = new SFTPFileCleaner();
    _ftpCleaner.Clean(_expectedRemotePath);
}
  1. What do you guys think about this design? Is it clean and easy to use?
  2. Can it be improved any further?
  3. Is there a name for this design pattern? To me is seems to be the mixture of Facade Pattern (Bastard Injection) and Strategy Pattern. Am I correct?
\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review! I see you added one line to the code, which doesn't appear to invalidate the current answer, but if the changes were more drastic then those would be reversed. For more information on this, see what you may and may not do after receiving answers. \$\endgroup\$ Commented May 24, 2018 at 18:35
  • \$\begingroup\$ Hi @SamOnela, sorry about that. I just realized I was missing a re-throw statement in my catch block. I didn't know about the rules. Won't happen again. :) \$\endgroup\$ Commented May 24, 2018 at 18:46
  • 1
    \$\begingroup\$ 1. Not terribly clean - you're mixing data processing (FTP file deletion) with user interface (Console.WriteLine) elements and therefore violating both Dependency Inversion and Single Responsibility Principle. Inject a logger (such as Log4Net's ILog) or at worse a TextWriter in the constructor for status logging operations. \$\endgroup\$ Commented May 24, 2018 at 19:20
  • \$\begingroup\$ @JesseC.Slicer, Is it a common practice to use Log4Net in test projects? I simply want to write to a console, so that I can see the error message in the NUnit test runner and debug things faster. I do have an ILogger interface in my production code with different implementations such as Log4Net and LocalModular (My own version). Can I do another Bastard Injection from the inherited classes and inject a NativeModularLogger(new ConsoleLogger()) into the RemoteFileCleaner ? \$\endgroup\$ Commented May 24, 2018 at 19:39

1 Answer 1

1
\$\begingroup\$

Wouldn't it make more sense to have IRemoteFileCleaner as the base class, and have FTPFileCleaner (which inherits from IRemoteFileCleaner as the class which all the others inherit from?

\$\endgroup\$
2
  • \$\begingroup\$ So, just a simple rename? Good idea. \$\endgroup\$ Commented May 24, 2018 at 16:10
  • \$\begingroup\$ @ShervinShahrdar Yeah, I guess so. \$\endgroup\$ Commented May 24, 2018 at 22:09

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.