Immutable File Upload Server Configuration Class (FTP or Network Drive)

Question

The application I'm working on needs to occasionally take pictures, and then send them to either an ftp server or a network drive (user configurable). To try to abstract away the differences between a ftp server and a network drive I made this class.

The idea is, the code that takes a picture with the camera and saves the image just needs to call either a WriteFTP or WriteLocal method with the URL from this class to get the image where it needs to go.

Since the class doing the picture taking and image writing is on a separate thread, I decided to make the server configuration immutable.

I feel like there's a better way to implement this, possibly using inheritance, because setting all of these configuration options in the constructor is rather cumbersome.

However I'm not sure if it would have been better to have four inherited classes, because really there's 4 kinds of saving options:

1. Local or network drive with a folder that can be specified at the time the file is saved (e.g., you could make a folder for each hour)

2. Local or network drive with a specified daytime and night time folder (e.g., "shift1", "shift2")

3. FTP folder that can be specified at the time the file is saved

4. FTP folder with a specified daytime and night time folder

---

// I considered having this be the base class, and having two classes inherit from it, e.g.,
// FTPDirectory : SaveDirectory, and
// LocalOrNetworkDirectory : SaveDirectory;
// 
// I am still not sure if that would have been better
internal class SaveDirectory
{
    private SaveDirectory(string description, LocationType type, bool usedynamicfolders, string dynamic_rootfolder, string custom_daytimefolder, string custom_nightfolder,
        string ftp_username, string ftp_password, Int16 ftp_port, string ftp_hostname)
    {
        this.Description = description;
        this.Type = type;
        this.UseDynamicFolders = usedynamicfolders;
        this.Dynamic_RootFolder = dynamic_rootfolder;
        this.Custom_Daytimefolder = custom_daytimefolder;
        this.Custom_NightFolder = custom_nightfolder;

        this.FTP_Username = ftp_username;
        this.FTP_Password = ftp_password;
        this.FTP_Port = ftp_port;
        this.FTP_Hostname = ftp_hostname;

        this.Verify();
    }

    // static factory methods. I don't like these. Usually I start to doubt what 
    // I'm doing when I need to use named parameters like this

    // I was thinking of using overloaded constructors but it
    // was very easy to accidentally forget one string and e.g. change this class from
    // a custom folder save location to a dynamic folder save location
    internal static SaveDirectory CreateUNC_DynamicFolderNames(string description, string rootfolder)
    {
        return new SaveDirectory(description: description, type: LocationType.UNC_Path,
                                usedynamicfolders: true,
                                dynamic_rootfolder: rootfolder,

                                custom_daytimefolder: null,
                                custom_nightfolder: null,

                                ftp_username: null,
                                ftp_password: null,
                                ftp_port: cNoPort,
                                ftp_hostname: null);
    }

    internal static SaveDirectory CreateUNC_CustomFolderNames(string description, string daytimefolder, string nightfolder)
    {
        return new SaveDirectory(description: description, type: LocationType.UNC_Path,
                                usedynamicfolders: false,
                                dynamic_rootfolder: null,

                                custom_daytimefolder: daytimefolder,
                                custom_nightfolder: nightfolder,

                                ftp_username: null,
                                ftp_password: null,
                                ftp_port: cNoPort,
                                ftp_hostname: null);

    }

    internal static SaveDirectory CreateFTPFolder_DynamicFolderNames(string description, string rootfolder, 
                                                                        string serverhostname, Int16 port, string username, string password)
    {
        return new SaveDirectory(description: description, type: LocationType.FTP,
                                usedynamicfolders: true,
                                dynamic_rootfolder: rootfolder,

                                custom_daytimefolder: null,
                                custom_nightfolder: null,

                                ftp_username: username,
                                ftp_password: password,
                                ftp_port: port,
                                ftp_hostname: serverhostname);
    }

    internal static SaveDirectory CreateFTPFolder_CustomFolderNames(string description, string daytimefolder, string nightfolder,
                                                                         string serverhostname, Int16 port, string username, string password)
    {
        return new SaveDirectory(description: description, type: LocationType.FTP,
                    usedynamicfolders: false,
                    dynamic_rootfolder: null,

                    custom_daytimefolder: daytimefolder,
                    custom_nightfolder: nightfolder,

                    ftp_username: username,
                    ftp_password: password,
                    ftp_port: port,
                    ftp_hostname: serverhostname);
    }

    private void Verify() // I put this in here because the constructor was long enough already
    {
        if (this.Type == LocationType.FTP)
        {
            if  (
                    (this.FTP_Username == null) ||
                    (this.FTP_Password == null) ||
                    (this.FTP_Port == cNoPort) ||
                    (this.FTP_Hostname == null)
                )
            {
                throw new ArgumentException("Internal error: Failed to create new ftp image saving location, incomplete or missing FTP server information.");
            }

        }

        if (this.UseDynamicFolders)
        {
            if (this.Dynamic_RootFolder == null)
            {
                throw new ArgumentException("Internal error: Failed to create new image saving location, missing root folder for dynamic directories.");
            }
        }
        else
        {
            if (
                    (this.Custom_Daytimefolder == null) ||
                    (this.Custom_NightFolder == null)
                )
            {
                throw new ArgumentException("Internal error: Failed to create new image saving location, missing at least one custom save directory.");
            }

        }


    }

    internal enum LocationType
    {
        // no reason for this to be 1 other than 0 being the default value for ints
        UNC_Path = 1,
        FTP 
    };

    // for a GUI (listview)
    internal string DisplayString
    {
        get
        {
            if (this.Type == LocationType.FTP)
            {
                return this.Description + "-- (" + this.FTP_Hostname + ")";
            }

            if (this.UseDynamicFolders)
            {
                return this.Description + "-- (" + this.Dynamic_RootFolder + ")";
            }
            else
            {
                return this.Description + "-- (" + this.Custom_Daytimefolder + ")";

            }
        }
    }

    internal static string CreateFTPURL(string hostname, Int16 port, string folder)
    {
        // I've run into a couple cases already where I forgot that I was naming my folders "folder/" instead of "folder" or "/folder" or "/folder/",
        // is there a better way?
        return "ftp://" + hostname + ":" + port.ToString() + "/" + folder;
    }

    // The FTP or local / network drive file writer function gets passed the output of this function.
    // it outputs something like \\myserver\myfolder\ or ftp://myserver:21/myfolder/
    internal string GetURL(string dynamic_daytimefolder, string dynamic_nightfolder, bool isdaytime)
    {

        if (this.Type == LocationType.FTP)
        {
            if (this.UseDynamicFolders)
            {
                if(isdaytime)
                {
                    return SaveDirectory.CreateFTPURL(this.FTP_Hostname, this.FTP_Port, dynamic_daytimefolder);
                }
                else
                {
                    return SaveDirectory.CreateFTPURL(this.FTP_Hostname, this.FTP_Port, dynamic_nightfolder);
                }
            }
            else
            {
                if(isdaytime)
                {
                    return SaveDirectory.CreateFTPURL(this.FTP_Hostname, this.FTP_Port, this.Custom_Daytimefolder);
                }
                else
                {
                    return SaveDirectory.CreateFTPURL(this.FTP_Hostname, this.FTP_Port, this.Custom_NightFolder);
                }
            }

        }
        else
        {
            if (this.UseDynamicFolders)
            {
                if(isdaytime)
                {
                    return this.Dynamic_RootFolder + dynamic_daytimefolder;
                }
                else
                {
                    return this.Dynamic_RootFolder + dynamic_nightfolder;
                }
            }
            else
            {
                if(isdaytime)
                {
                    return this.Custom_Daytimefolder;
                }
                else
                {
                    return this.Custom_NightFolder;
                }
            }
        }


    }

    // this is an immutable object,
    // is there any reason to not make these public/internal if they are readonly?
    internal readonly string Description;
    internal readonly LocationType Type;
    internal readonly bool UseDynamicFolders;
    internal readonly string Dynamic_RootFolder;

    internal readonly string Custom_Daytimefolder;
    internal readonly string Custom_NightFolder;

    internal readonly string FTP_Hostname;

    internal const Int16 cNoPort = 0;
    internal const Int16 cDefaultFTPPort = 21;

    internal readonly Int16  FTP_Port;
    internal readonly string FTP_Username;
    internal readonly string FTP_Password;       

}

EDIT: Note that each image write can have a different SaveDirectory configuration, and each configuration is different, e.g., one of them could go to ftp://myserver/myfolder/ and one could go to \\someotherserver\folder\. SaveDirectory must take in the configuration when it is constructed, this configuration comes from a (Windows Forms) GUI window.

Answer 1

The huge constructore is the result that the SaveDirectory class is doing too much. It has to know everything about the ftp server as well as about the network drive. You need to separate them.

I suggest the following structure:

Define a storage interface:

interface IImageStorage
{
    bool SaveImage(Image image);
    // if there are any other common methods or properties
    // you can define them here too
}

Create two storages that implement the interface:

class FtpImageStorage : IImageStorage
{
    // the constructor and
    // all ftp properties go into this class

    public bool SaveImage(Image image) { return false; }
}

class NetworkDriveImageStorage : IImageStorage
{
    // the constructor and
    // all network drive properties go into this class

    public bool SaveImage(Image image) { return false; }
}

Extract the enum type from the class to not be nested:

internal enum LocationType
{
    // no reason for this to be 1 other than 0 being the default value for ints
    UNC_Path = 1,
    FTP
};

Create a storage factory that decides based on the parameter which storage to create and knows how to initialize each one. The settings come from a configuration I guess?

class ImageStorageFactory
{
    // register the storages here
    private static readonly Dictionary<LocationType, Func<IImageStorage>> _createStorageFuncs =
        new Dictionary<UserQuery.LocationType, Func<IImageStorage>>
        {
            [LocationType.UNC_Path] = CreateNetworkDriveImageStorage,
            [LocationType.FTP] = CreateFtpImageStorage,
        };

    // you can also name the method GetImageStorage and cache them if necessary
    public static IImageStorage CreateImageStorage(LocationType locationType)
    {
        return _createStorageFuncs[locationType]();
    }

    private static IImageStorage CreateFtpImageStorage()
    {
        // ftp image storage initialization
        return new FtpImageStorage();
    }

    private static IImageStorage CreateNetworkDriveImageStorage()
    {
        // network drive storage initialization
        return new NetworkDriveImageStorage();
    }
}

Now you can easily add more storages anytime and you can test the ones you already have without affecting the others. Their constructors require less parameters and are easier to use.