2
\$\begingroup\$

I'm struggling to understand how to implement principles of encapsulation, inheritance and polymorphism to break this code into logical parts. My background is in VBS/Classic ASP and database development, not OOP.

I'm not asking anyone to write the code, but rather to point me in the right direction. I have this class, which I suppose is something I've heard referred to as a God Object.

My code works, but it's unmanageable and will only get worse.

This class:

  1. Receives information about a data source, like the path to an Access database or the name of a db server.
  2. Determines which engine can process the file... Access, MSSQL, MySQL...
  3. Select a Provider to connect to the DBMS
  4. Set db specific settings, like whether to use single or double quotes around object names, etc.
  5. Generate a connection string to connect to the db.

Is it Ok to have all this stuff in a single class?

I was thinking it would be easier to understand later, if I separated it into a few different classes:

  1. A class to figure out what kind of data source it will connect to
  2. A class to set db specific settings and generate a connection string

Does that sound right? If so, would you recommend maybe pulling all of these enums and their accompanying methods into a separate static class so that they don't need to be instantiated?

using System;
using System.Diagnostics;
using System.IO;

namespace RickRoll
{
public class Db
{
    public string DataSource { get; }
    public string Database { get; }
    public string Username { get; }
    public string Password { get; }
    public string ConnectionString { get; private set; }
    public string ExtendedProperties { get; }

    private WrapColumn wrapcol;
    private WrapObject wrapobj;

    public DataProvider Provider { get; set; }
    public DataFileType FileType { get; private set; }
    public DataEngine Engine { get; private set; }
    public WrapColumn WrapCol { get => wrapcol; private set => wrapcol = value; }
    public WrapObject WrapObj { get => wrapobj; private set => wrapobj = value; }

    // This can't be right, but I needed to do it to get the enun values (temporary fix)
    public Db()
    {
        // only used for public enum access
    }

    // For file databases without password set
    public Db(string filepath)
    {
        FileType = GetDbFileType (filepath);
        Engine = GetDbEngine (FileType);
        Provider = GetDbProvider (FileType);
        DataSource = filepath;
        Database = Path.GetFileName (filepath);
        Username = null;
        Password = null;
        ConnectionString = GetConnectionString (Provider);          
        GetWrapCol (Engine);
        GetWrapObj (Engine);
    }

    // For file databases with password set
    public Db(string filepath, string password)
    {
        FileType = GetDbFileType (filepath);
        Engine = GetDbEngine (FileType);
        Provider = GetDbProvider (FileType);
        DataSource = filepath;
        Database = Path.GetFileName (filepath);
        Username = null;
        Password = password;
        ConnectionString = GetConnectionString (Provider);
        GetWrapCol (Engine);
        GetWrapObj (Engine);
    }

    // For all connections that use Windows AUthentication or no password 
    public Db(string path, string server , string database)
    {
        FileType = GetDbFileType (path);
        Engine = GetDbEngine (FileType);
        Provider = GetDbProvider (FileType);
        DataSource = GetDataSource (Engine , path , server);
        Database = database;
        Username = null;
        Password = null;
        ConnectionString = GetConnectionString (Provider);
        GetWrapCol (Engine);
        GetWrapObj (Engine);
        Debug.WriteLine (ConnectionString);
        Debug.WriteLine (Environment.UserName);
    }

    // For all connection types 
    public Db(string path, string server, string database , string username , string password)
    {
        FileType = GetDbFileType (path);
        Engine = GetDbEngine (FileType);
        Provider = GetDbProvider (FileType);
        DataSource = GetDataSource (Engine, path, server);
        Database = database;
        Username = username;
        Password = password;
        ConnectionString = GetConnectionString (Provider);
        GetWrapCol (Engine);
        GetWrapObj (Engine);
    }

    // For instantiating from data stored in SQL Server using Windows Authentication or no password
    public Db(int filetype_value, string server , string database)
    {
        FileType = GetDbFileType (filetype_value);
        Engine = GetDbEngine (FileType);
        Provider = GetDbProvider (FileType);
        DataSource = server;
        Database = database;
        Username = null;
        Password = null;
        ConnectionString = GetConnectionString (Provider);
        GetWrapCol (Engine);
        GetWrapObj (Engine);
        Debug.WriteLine (ConnectionString);
        Debug.WriteLine (Environment.UserName);
    }

    // For instantiating from data stored in SQL Server using SQL Authentication or JET DB password
    public Db(int filetype_value , string server , string database , string username , string password)
    {
        FileType = GetDbFileType (filetype_value);
        Engine = GetDbEngine (FileType);
        Provider = GetDbProvider (FileType);
        DataSource = server;
        Database = database;
        Username = username;
        Password = password;
        ConnectionString = GetConnectionString (Provider);
        GetWrapCol (Engine);
        GetWrapObj (Engine);
    }

    /// <summary>
    /// This changes the database connection string to use a different Provider.
    /// </summary>
    /// <param name="p"></param>
    public void ReSetConnectionString(DataProvider p)
    {
        Provider = p;
        ConnectionString = GetConnectionString ( Provider );
    }

    // Gets file extension from file name or an ext can be passed in place of path
    private DataFileType GetDbFileType(string path)
    {
        DataFileType ft;
        string ext;
        if (path.Contains ("."))
        {  
                 // allows for attached databases 
            string fpath = path.Replace ("|DataDirectory|" , AppDomain.CurrentDomain.BaseDirectory);
            ext = Path.GetExtension (fpath).Replace ("." , "").ToUpper ( );

            if (Enum.TryParse<DataFileType> (ext , out ft) == true) return ft;
        }
        else
        {
            ft = (DataFileType) Enum.Parse (typeof (DataFileType) , path.ToUpper());            /// throw exception if extention is not in DataFileType enum or is null
        }
        return ft;
    }

   // Returns the path to a file for MS Access, etc or a db server name
    private string GetDataSource(DataEngine e, string path, string server)
    {
        if (DataSourceIsFile (e)) return path;
        return server;
    }
   // I think this is unnecessary.  Use this instead:  DataFileType dft = (DataFileType) value;
    private DataFileType GetDbFileType(int value)
    {
        DataFileType ft = (DataFileType) Enum.Parse (typeof (DataFileType) , value.ToString ( ));        // throws and exception if int isn't in the DataFileType enum.
        return ft;
    }

  // This is the db engine
    public enum DataEngine
    {
        None,
        ACCESS = 1,              // 0 = not applicable or not determined
        MSSQL,
        EXCEL,
        ORACLE,
        MYSQL,
        TEXT
    }

    // <summary>
    /// These are the int enum values that get stored in the database for each type of data file
    /// </summary>
    public enum DataFileType
    {
        None,
        MDB = 1,       // 0 = not a configured data file
        ACCDB,
        MDF,           // Primary Data FIle
        NDF,           // File Group (secondary data files)
        XLS,           // Excel 97-2003 worksheet
        XLSX,          // Excel 2007 workbook
        XLSXM,         // Macro enabled workbook
        XLTM,          // Binary worksheet (BIFF12)
        XLW,           // Excel works space, previously known as workbook
        CSV,           // Comma separated values
        TAB,           // Tab separated values
        TSV,           // Tab separated values
        TXT            // Delimited Text file
    }

    /// <summary>
    /// These are the int values that get stored in the database for each db connection
    /// </summary>
    public enum DataProvider
    {
        None,
        Microsoft_ACE_OLEDB_12_0 = 1,          // Microsoft.ACE.OLEDB.12.0 - MS OLEDB DataProvider for MDB or ACCDB or EXCEL
        Microsoft_ACE_OLEDB,               // Microsoft.ACE.OLEDB VersionIndependentProgID
        Microsoft_Jet_OLEDB_4_0,           // MS Access - Does not work with ACCDB or any SQL Server version
        Microsoft_Jet_OLEDB,               // Version Independent ProgID
        SQLNCLI11,                         // SQL Server Native Client for OleDb
        SQLNCLI,                           // Version Independent ProgID
        SQLOLEDB_1,                        // SQL Server OleDb - Does not work with SQL Server Express
        SQLOLEDB,                          // VersionIndependentProgID
        SQL__Server__Native__Client__11_0, // SQL Server Native Client using ODbC
        SQL__Server__Native__Client,       // Version Independent ProgID
        MSDASQL_1,                         // Microsoft OleDb Data Access Components using ODbC
        MSDASQL                            // Version Independent ProgID
    }

  // This can be simplified
    private DataEngine GetDbEngine(int enumvalue)
    {
        DataEngine result = (DataEngine) Enum.Parse (typeof (DataEngine) , enumvalue.ToString ( ));        // throws and exception if int isn't in the DataEngine enum.
        return result;
    }
   // Gets the Database Engine from type of file
    private DataEngine GetDbEngine(DataFileType ft)
    {
        switch (ft)
        {
            case DataFileType.MDB:
                return DataEngine.ACCESS;
            case DataFileType.ACCDB:
                return DataEngine.ACCESS;
            case DataFileType.MDF:
                return DataEngine.MSSQL;
            case DataFileType.NDF:
                return DataEngine.MSSQL;
            case DataFileType.XLS:
                return DataEngine.EXCEL;
            case DataFileType.XLSX:
                return DataEngine.EXCEL;
            case DataFileType.CSV:
                return DataEngine.TEXT;
            case DataFileType.TAB:
                return DataEngine.TEXT;
            case DataFileType.TSV:
                return DataEngine.TEXT;
            case DataFileType.TXT:
                return DataEngine.TEXT;
            default:
                throw new ArgumentException ($"* * * DataFileType is not a supported data file format.  Database DataEngine could not be determined.");
        }
    }


  // Returns the database provider to use for each supported file type
    private DataProvider GetDbProvider(DataFileType ft)
    {
        switch (ft)
        {
            case DataFileType.MDB:
            case DataFileType.ACCDB:
                return DataProvider.Microsoft_ACE_OLEDB_12_0;
            case DataFileType.MDF:
                return DataProvider.SQLNCLI11;                 // SQLOLEDB_1 and SQLOLEDB did not work with SQL Server Express
            case DataFileType.NDF:
                return DataProvider.SQLNCLI11;
            case DataFileType.XLS:
            case DataFileType.XLSX:
            case DataFileType.CSV:
            case DataFileType.TAB:
            case DataFileType.TSV:
            case DataFileType.TXT:
                return DataProvider.Microsoft_ACE_OLEDB_12_0;
            default:
                throw new ArgumentException ($"* * * DataFileType is not a supported data file format.  Database DataProvider could not be determined.");
        }
    }

    // Sets the character used to 'wrap' column names in query strings in case they have spaces or are reserved words
    private void GetWrapCol(DataEngine e)
    {
        switch (e)
        {
            case DataEngine.ACCESS:
                WrapCol = new WrapColumn  { left = "`" , middle = "`,`" , right = "`" };
                break;
            case DataEngine.MSSQL:
                WrapCol = new WrapColumn  { left = "[" , middle = "],[" , right = "]" };
                break;
            case DataEngine.EXCEL:
                WrapCol = new WrapColumn  { left = "`" , middle = "`,`" , right = "`" };
                break;
            case DataEngine.ORACLE:
                WrapCol = new WrapColumn  { left = @"""" , middle = @""",""" , right = @"""" };
                break;
            case DataEngine.MYSQL:
                WrapCol = new WrapColumn  { left = "`" , middle = "`,`" , right = "`" };        // might be brackets for sysnames and ` for columns
                break;
            case DataEngine.TEXT:
                WrapCol = new WrapColumn  { left = @"""" , middle = @""",""" , right = @"""" };        // not sure how to handle this yet
                break;
            default:
                throw new ArgumentException ($"* * * DataEngine was not valid. Could not determine a COLUMN escape character.");
        }
    }

        // Sets the character used to 'wrap' object names (table names, schema names, etc.) in query strings in case they have spaces or are reserved words
    private void GetWrapObj(DataEngine e)
    {
        switch (e)
        {
            case DataEngine.ACCESS:
                WrapObj = new WrapObject { left = "`" , middle = "`,`" , right = "`" };
                break;
            case DataEngine.MSSQL:
                WrapObj = new WrapObject { left = "[" , middle = "],[" , right = "]" };
                break;
            case DataEngine.EXCEL:
                WrapObj = new WrapObject { left = "`" , middle = "`,`" , right = "`" };
                break;
            case DataEngine.ORACLE:
                WrapObj = new WrapObject { left = @"""" , middle = @""",""" , right = @"""" };
                break;
            case DataEngine.MYSQL:
                WrapObj = new WrapObject { left = "[" , middle = "],[" , right = "]" };        // might be brackets for sysnames and ` for columns
                break;
            case DataEngine.TEXT:
                WrapObj = new WrapObject { left = @"""" , middle = @""",""" , right = @"""" };        // not sure how to handle this yet
                break;
            default:
                throw new ArgumentException ($"* * * DataEngine was not valid. Could not determine a OBJECT escape character.");
        }
    }

    // Returns false for server bases DBMS and true for file based Db's like Access
    private bool DataSourceIsFile(DataEngine e)
    {
        switch (e)
        {
            case DataEngine.ACCESS:
                return true;
            case DataEngine.MSSQL:
                return false;
            case DataEngine.EXCEL:
                return true;
            case DataEngine.ORACLE:
                return false;
            case DataEngine.MYSQL:
                return false;
            case DataEngine.TEXT:
                return true;
            default:
                throw new ArgumentException ($"* * * DataEngine was not valid. Could not determine if this is a file or server DataSource.");
        }
    }

    // Connection strings for any of the supported Providers

    //https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/connection-string-syntax?view=netframework-4.7.1
    private string GetConnectionString(DataProvider Provider)
    {
        string ProgId = Provider.ToString ( ).Replace ("_" , ".");
        string result = "";
        switch (Provider)
        {
            case DataProvider.Microsoft_ACE_OLEDB_12_0:                                                           // Microsoft MS OLEDB for MDB or ACCDB or EXCEL
            case DataProvider.Microsoft_ACE_OLEDB:                                                                // VersionIndependentProgID
                return $@"Provider={ProgId};Data Source={DataSource};Persist Security Info=False;Jet OLEDB:Database Password={Password.EmptyIfNull ( )};";

            case DataProvider.Microsoft_Jet_OLEDB_4_0:                                                            // MS Access Jet OLEDB - Does not work with ACCDB or any SQL Server version
            case DataProvider.Microsoft_Jet_OLEDB:                                                             // VersionIndependentProgID
                // Jet db with user-lvel security requires a Workgroup information file designation:
                // Jet OLEDB:System Database=|DataDirectory|\System.mdw;"   <--- that could be stored in the Server field
                // Jet user-level security uses the User ID and Password setting.  A new constructer that includes the Username field will need to be added to support this 
                if (Engine == DataEngine.ACCESS && Password.NullIfEmpty() == null)
                    return $@"Provider={ProgId};Data Source={DataSource};User ID=Admin;Password=;";
                else
                    return $@"Provider={ProgId};Data Source={DataSource};Persist Security Info=False;Jet OLEDB:Database Password={Password.EmptyIfNull ( )};";


            case DataProvider.SQLNCLI11:                                                                          // SQL Server OleDb Native Client
            case DataProvider.SQLNCLI:                                                                            // VersionIndependentProgID
                result = $@"Provider={ProgId};Server={DataSource};Database={Database};";
                if (Password != null) result += $"Uid={Username.EmptyIfNull ( )};Pwd={Password.EmptyIfNull ( )};";
                if (Password == null) result += "Integrated Security=SSPI;";        
                return result;

            case DataProvider.SQLOLEDB_1:                                                                         // Microsoft OLE DB DataProvider for SQL Server (I've seen this work for ACCESS also) - DID NOT work for SQL Server 2016 Express, but DID work for SQL Server 2016 Developer Edition
            case DataProvider.SQLOLEDB:                                                                           // VersionIndependentProgID
                result = $@"Provider={ProgId};Data Source={DataSource};Initial Catalog={Database};";
                if (Password != null) result += $"User Id={Username.EmptyIfNull()};Password={Password.EmptyIfNull ( )};";
                if (Password == null) result += "Integrated Security=SSPI;";                                                                                            
                return result;

            case DataProvider.MSDASQL_1:                                                                          // Microsoft Data Access OleDb using ODbC
            case DataProvider.MSDASQL:                                                                            // VersionIndependentProgID
                if (Engine == DataEngine.ACCESS)
                    return $@"Driver={{Microsoft Access Driver (*.mdb, *.accdb)}};DbQ={DataSource}";
                if (Engine == DataEngine.EXCEL)
                    return $@"Driver={{Microsoft Excel Driver (*.xls, *.xlsx, *.xlsm, *.xlsb)}};DbQ={DataSource}";
                if (Engine == DataEngine.MSSQL)
                    if (string.IsNullOrEmpty (Username))
                        return $@"[DataProvider = MSDASQL;] DRIVER={{SQL Server}}; SERVER={DataSource}; DATABASE={Database};UID=;Integrated Security=SSPI";
                    else
                        return $@"[DataProvider = MSDASQL;] DRIVER={{SQL Server}}; SERVER={DataSource}; DATABASE={Database};UID={Username};PWD={Password}";
                else
                    throw new ArgumentException ($" * * * The MSDASQL Provider has only been set up for MS Access or Excel or MSSQL connections.  Could not create Connection String");


            case DataProvider.SQL__Server__Native__Client__11_0:                                        // SQl Server OleDb using ODbC
                if (Engine == DataEngine.MSSQL && (Username.NullIfEmpty ( ) != null))
                    return $@"Driver={{{ProgId}}};Server={DataSource};Database={Database};Uid={Username};Pwd={Password};";
                else
                    return $@"Driver={{{ProgId}}};Server={DataSource};Database={Database};Trusted_Connection=yes;";

            default:
                throw new ArgumentException ($" * * * DataProvider is not valid.  Could not create Connection String");
        }
    }

    public string Columns(string value)
    {
        string[ ] c = value.Split (',');                                                        // if col names have "," in them, maybe replace it with another char and modify this method to replace it back.
        return $"{WrapCol.left}{string.Join(WrapCol.middle , c)}{WrapCol.right}";
    }



}


public static class StringExt
{
    public static string NullIfEmpty(this string value)
    {
        return string.IsNullOrEmpty (value) ? null : value;
    }

    public static string EmptyIfNull(this string value)
    {
        if (string.IsNullOrEmpty (value))
            return string.Empty;
        else
            return value;
    }

}
}
\$\endgroup\$
  • 1
    \$\begingroup\$ @Heslacher, Done. \$\endgroup\$ – rbsdca Feb 14 '18 at 5:27
  • \$\begingroup\$ // only used for public enum access how do you mean that? Could you add an example? \$\endgroup\$ – t3chb0t Feb 14 '18 at 8:48
  • \$\begingroup\$ @t3chb0t, I ended up persisting the enumerations in Int32 columns in SQL Server and so I needed to be able to check if (reader.GetSqlInt32(datafiletype) == (int) Db.DataFileType.MDB) for example, without calling any of the constructors that actually create the database connection. I just let that in there to illustrate how poorly conceived the class was. \$\endgroup\$ – rbsdca Feb 14 '18 at 12:37
  • \$\begingroup\$ I also wanted to illustrate that I do need public access to the DataEngine, DataFileType and DataProvider enums \$\endgroup\$ – rbsdca Feb 14 '18 at 12:44
5
\$\begingroup\$

Quick feedback:

I'll elaborate on the important parts afterwards.

  • The code sample you posted is long and confusing. This isn't an insult to whoever wrote it; but it is a first indication that you're going to need to break this up more.
  • If you quickly browse through the posted code, you can quickly see a copy/paste template emerge. The constructors, the repeated switch statements, ... these are all indications that your code is not sufficiently implementing reusable patterns.
  • All in all, you need to observe SOLID more than you currently have.
  • Instead of using an enum for listing every type of data access (which means you're going to have a switch case somewhere to differentiate the needed code flow), you need to defines classes that do the work. This is the OOP way of doing things. Instead of developing functions that do the correct operations (based on an enum), you need to develop objects that do the specific work you want them to.

OOP basics:

This might help explain the intention of OOP and how you should implement it. Let's start from a simplified example:

A Human needs to be created. A human has a head, torso, and set of legs. Any combination of a head, torso and legs is a valid human.

  • Head options:

    • Human head
    • Gorilla head
    • Bionic head
  • Torso options:

    • Human torso
    • Gorilla torso
    • Cyborg torso
  • Leg options:

    • Peg legs
    • Human legs
    • Cheetah legs
    • Cyborg legs

The developer of the posted code is likely already thinking of using switch cases to distinguish between these different types of heads/torsos/legs.

The correct OOP approach would be something like this:

public class Human
{
    public Head Head { get; set; }
    public Torso Torso { get; set; }
    public Legs Legs { get; set; }

    public Human(Head head, Torso torso, Legs legs)
    {
         this.Head = head;
         this.Torso = torso;
         this.Legs  = legs;
    }

    public string CommunicateMessage()
    {
        return this.Head.SaySomething();
    }
}

The head options are then defined by deriving from the common Head class definition:

public enum Material { Flesh, Metal }

public abstract class Head
{
    public Material Material { get; }
    public bool IsFaceHairy { get; }

    public abstract string SaySomething();
}

And then you implement the options in the derived classes:

public class HumanHead : Head
{
    public HumanHead()
    {
        this.Material = Material.Flesh;
        this.IsFaceHairy = false;
    }

    public override string SaySomething()
    {
        return "Hello!";
    }
}

public class GorilllaHead : Head
{
    public GorilllaHead()
    {
        this.Material = Material.Flesh;
        this.IsFaceHairy = true;
    }

    public override string SaySomething()
    {
        return "OO OO AH AH AH!";
    }
}

public class BionicHead : Head
{
    public BionicHead()
    {
        this.Material = Material.Metal;
        this.IsFaceHairy = false;
    }

    public override string SaySomething()
    {
        return "YOU WILL BE UPGRADED!";
    }
}

The same is true for Torso and Legs. I've omitted this code for brevity's sake.

This allow you to create humans however you want, by assembly them as you please:

Human tom = new Human(new GorillaHead(), new HumanTorso(), new HumanLegs());
Human george = new Human(new HumanHead(), new GorillaTorso, new CheetahLegs());
Human cyberman = new Human(new BionicHead(), new CyborgTorso(), new CyborgLegs());

The important part to take away from this example is that every Head is able to communicate the same way. After assembling the human, the subsequent code does not need to care anymore which particular head this human has:

string message1 = tom.CommunicateMessage();
string message2 = george.CommunicateMessage();
string message3 = cyberman.CommunicateMessage();

No need for enums, switch cases, else ifs, or anything else. This is the goal of an OOP approach. By creating proper class definitions, you simplify the future code that will have to utilize these classes.

Some slightly more advanced comments about the OOP approach I used:

  • I've declared the Head class as abstract so that it's impossible to create a new Head(). Abstract classes cannot be instantiated, only their deriving classes can be instantiated (if they're not abstract themselves).
  • I opted for setting the properties in the constructor of the deriving class. There are other ways of doing this, I simply picked the one that is closest to what you already know (since you heavily relied on constructors in the posted code)
  • You can use interfaces instead of abstract classes. Again, I simply picked the easier example and am forgoing a discussion on the use of interfaces.
  • My example is oversimplified for the sake of clarity. There are many, many different OOP approaches. You should really delve into this deeper if you want to make an informed decision on how to best approach this. I'm only trying to give you a first example of why OOP is beneficial for your situation.

Splitting responsibilities

Note: I've oversimplified your example to only differentiate between a handful of data source types, for brevity's sake.

The S in SOLID is the Single Responsibility Principle. More information here, the gist of it is that no class definiton should ever contain more than one responsibility.

From your question:

This class:

  1. Receives information about a data source, like the path to an Access database or the name of a db server.
  2. Determines which engine can process the file... Access, MSSQL, MySQL...
  3. Select a Provider to connect to the DBMS
  4. Set db specific settings, like whether to use single or double quotes around object names, etc.
  5. Generate a connection string to connect to the db.

Is it Ok to have all this stuff in a single class?

It is not okay, because it violates the Single Responsibility Principle. As I currently understand your situation, there are a few distinct responsibilities:

  • A - Deciding which type of data source is being referenced
  • B - Retrieving the data from the source
  • C - Providing the retrieved data to the external caller

Notice the letters, I will reuse these across the example.

The existence of these 3 responsibilities means that you will need at least 3 classes, in order to not violate SRP. Spoiler alert, you'll need more than 3 classes, but we'll get to that in a minute.


A is relatively simple. It takes some parameters (the parameters you're now passing in the different constructors), and it figures out which data provider will be used. Ideally, the return value of this method/class should be of type B.

Alternatively, it can also return a value of type C, provided that this C object will contain the correct B object as one of its properties.

The core intention here is to provide a single class that takes the parameters and evaluates them accordingly. After this evaluation has been completed, the future code should no longer contain forks such as switch cases or if blocks intended to differentiate between different types of providers.


C is similarly simple. It exposes methods to external callers, and will perform operations on an internal B property to retrieve the data that the external caller requests.

The core intention here is to provide a single class that can work with any B. This is similar to the Human class from the example. Every Human can be operated (by external callers) the same way, regardless of which head/torso/legs this human has.
Similarly, every C should be able to be operated the same way by an external caller, regardless of whether the underlying data source is an SQL db, an Excel file or an Access file.


B is more complicated. Remember the Head example from the OOP basics? B is essentially going to be the same: a base class that's intended to be inherited.
In the interest of using clear names, I'll rename B to a more understandable class name: DataSource (not to be confused with the existing .NET class of the same name!)

Just like how we derived Head for every type of head that existed (HumanHead,GorillaHead,BionicHead), we're going to have to derive DataSource for every type of source that exists: ExcelDataSource, SQLDataSource, AccessDataSource, ...

This is where things get hairy. Since your code is differentiating between different types of Excel file, you might end up with multilevel inheritance. Something like this (indentation = inheritance):

  • DataSource
    • ExcelDataSource
      • XlsDataSource
      • XlsxDataSource
      • XlsxmDataSource
    • AccessDataSource
    • SQLDataSource

There are also a few other considerations, such as e.g. having ExcelDataSource and AccessDataSource derived from a common FileDataSource; whereas letting SqlDataSource and MySqlDataSource derive from a common NetworkDataSource. These are all possible suggestions. You need to pick the one that suits your needs, and this answer can't delve into these nitty gritty details.

The main benefits of such an approach:

  • You can avoid copy/paste code by implementing the logic on the correct level of the inheritance tree:
    • If all Excel data sources (xls, xlsx, xlsm, ...) have a shared logic (e.g. checking if the file exists), you only need to implement this logic in the shared class (ExcelDataSource.FileExists())
    • If a specific Excel file (e.g. xlsx) has a unique piece of logic that does not apply for any other type of Excel file, then you can implement this logic in the specific class (XlsxExcelDataSource)
    • If you're implementing a very similar piece of logic in multiple classes, there's a good chance that you need to put this logic in the base class that these classes derive from. This avoids duplicate code.
  • Your C object will have a DataSource object. It's important to realize that the C object is not allowed to know/care which particular derived class of DataSource is being used. This allows C to be B-agnostic (= encapsulation, major OOP principle).
  • Inheritance can provide additional type safety, e.g. you can limit a particular method to only accept e.g. ExcelDataSource objects instead of e.g. pointlessly also acception SqlDataSource objects. When using enums, this was not possible without having to manually check for the enum type and throw a runtime exception (as opposed to a compile-time error, which is much better for the developer).
  • If you suddenly have to add new data sources (or remove existing ones), the cost of implementing this change is minimized. You simply need to derive a new xxxxxDataSource class (independent of the existing classes), and need to update the logic in A to account for this new class' existence in its evaluation logic.
    • In the old system, you would've had to create a new enum value and then update all switch cases that evaluate this enum.
    • If you forgot to update one or two switch cases, they would use their default case (without throwing an error), and you'll end up with unexpected behavior that is going to be annoying to debug (you'd have to step through every switch to see what was happening).

Conclusion
I can't give you the conclusive answer because it's highly dependent on your situation. But the intention and goal should be clear: Instead of using enums, you should be using class definitions and inheritance. This allows you to maximize code reusability, avoid copy/paste issues, and avoids unnecessary switch or if blocks.


How to start implementing this?

This is a very rudimentary example to show you how you can start splitting up the existing class. If your old class looks like this:

public class OldExampleDataSource
{
    private ConnectionType MyConnectionType { get; set; }

    public OldExampleDataSource(string a)
    {
        //SQL constructor logic
    }

    public OldExampleDataSource(bool b)
    {
        //Excel constructor logic
    }

    public void DoSomething()
    {
        switch (MyConnectionType)
        {
            case ConnectionType.SQL:
                //SQL oepration logic
            case ConnectionType.Excel:
                //Excel operation logic
            default:
                break;
        }
    }
}

You need to group the logic in separate classes, based on the connection type:

public class DataSource
{
    public DataSource()
    {
         //Default constructor logic
    }

    public virtual void DoSomething()
    {
        //Default operation logic
    }
}

public class SqlDataSource : DataSource
{
    public SqlDataSource()
    {
        //SQL constructor logic
    }

    public void DoSomething()
    {
        //SQL operation logic
    }
}

public class ExcelDataSource : DataSource
{
    public ExcelDataSource()
    {
        //Excel constructor logic
    }

    public void DoSomething()
    {
        //Excel operation logic
    }
}

Here, you see the main benefit appear: even though you've retained all the constructor and operation logic, you've eliminated the need for the enum and te subsequent switch cases.

\$\endgroup\$
  • \$\begingroup\$ This is good stuff. I'll be studying it throughout the coming days. Why I chose enums: I was looking for a way to express "supported file types, by file extension" in a way that wouldn't involve ToUpper() because I thought that looked ugly. Int has a smaller footprint in DB as well. Also, I wanted to learn how to use enums. I maybe got carried away. lol Also, I didn't create a class for every data provider because much of what it does is exactly the same for every one... makes me think I need to study more about inheritance. NOTE: I have not finished reading it. \$\endgroup\$ – rbsdca Feb 15 '18 at 12:59
  • \$\begingroup\$ @rbsdca (1) Why I chose enums: I was looking for a way to express "supported file types, by file extension"I would use a string[] or List<string> (2) Also, I wanted to learn how to use enums. I maybe got carried away. lol Experience mostly comes from making mistakes and learning from them :) No worries. (3) much of what it does is exactly the same for every one The idea of inheritance is to put shared operations/logic in a shared base class, so you don't have to duplicate code. A good inheritance/OOP structure prevents duplicate code from forming. \$\endgroup\$ – Flater Feb 15 '18 at 13:23
  • \$\begingroup\$ I'm emotionally invested in using the enum for my file types, but I see you walking towards me with a lit torch and I really dislike switch statements. What sent me down this path is reading that Classes, not Methods are the functional components of OOP. After programming for 10 years in VBS where everything's a method... it's hard to wrap my head around that. \$\endgroup\$ – rbsdca Feb 15 '18 at 14:13
  • \$\begingroup\$ @rbsdca: I can understand the difficulty in wrapping your head around it. I think the easiest way to translate between the two is to change the grammar (changing the verb into a noun). Using a cooking recipe as an example: you can either think of it as mixing the batter (method-based thinking) or putting the batter in the mixer (object-based thinking). OOP is all about trying to make a mixer that can be used afterwards. Especially if you have to mix many things , it's better to build one mixer instead of manually mixing things every time. \$\endgroup\$ – Flater Feb 15 '18 at 15:09
  • 1
    \$\begingroup\$ @rbsdca: Link it in a comment and I'll have a look if you want. \$\endgroup\$ – Flater Feb 21 '18 at 12:14
1
\$\begingroup\$

Too much for a comment.

It seems really broad set of data sources to think you can have a common set of commands / queries.

If you go with .NET data provides you get a common set of methods and properties via DbCommand Class. For example DataReader.

You can find 3rd party providers for a lot of sources. Many sources (I think Excel is one) support OLEDB or ODBC. I have not used one but I have seen .NET data providers for CSV.

\$\endgroup\$
  • \$\begingroup\$ It's working as designed, but what I was hoping for, is some advice regarding how to rewrite the class such that it adheres to OOP principles. In it's current state, it will become difficult to debug very quickly. \$\endgroup\$ – rbsdca Feb 15 '18 at 6:48
  • \$\begingroup\$ My class actually does use all .NET data providers. The assembly needs to connect to all of them, but you're right, it all doesn't need to to happen in the same class. I want it to search my hard drive/connect to DB Servers on my dev bix and display all of the databases/files in a tree, so that I can click to view data from say, mySQL, MSSQL, Access and Excel, side by side instead of having to search for databases, open them in DBMS and switch between them. I know I can use OPENROWSET, etc, but I also don't want to have to do any ETL on the data either. \$\endgroup\$ – rbsdca Feb 16 '18 at 18:19

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.