Code organization for .NET solution

I am going through one of my class library projects, and while nothing is wrong with it, I am finding myself being a bit anal, and wanting to organize things a bit differently.

The project in particular that I am looking at reorganizing, is a class library to hook into a multitude of databases, and do work on them. Now because of the differences in datatypes and methods for doing said work, each database has its own sub-folder in the main project, labelled after the database it will do the work for.

Would it be best to keep it like it is, or combine the similar methods into 1 class and simply through a switch statement in, giving the option to "select" the database type the library should be working with?

Right now it's setup as you simply use or import My.ClassLibrary.MSSQL and the like, however, all the methods in each type are so flippin similar, I am looking at it and saying to myself... why?


Each Database type has a folder in this project containing the database specific representation of the following code:

Access Class

```
using System;
using System.Data;
using System.Data.SqlClient;
using System.Threading.Tasks;

namespace kp.Class.Library.Data
{
internal class Access : IDisposable
{

#region "Properties"

// Set the type of query we are running
private CommandType _QT;
internal CommandType QueryType { set { _QT = value; } }

// Set the actual query text to run
private string _Qry;
internal string Query { set { _Qry = value; } }

// Set the parameter names if there are any
private string[] _PNs;
internal string[] ParameterNames { set { _PNs = value; } }

// Set the parameter values if there are any
private object[] _PVs;
internal object[] ParameterValues { set { _PVs = value; } }

// Set the actual Sql Data Types if there are any
private DataTypes[] _DTs;
internal DataTypes[] ParameterDataTypes { set { _DTs = value; }

Would it be best to keep it like it is, or combine the similar methods
into 1 class and simply through a switch statement in, giving the
option to "select" the database type the library should be working
with?

This is a two part question. Where I am only refering to

Part1


Would it be best to keep it like it is, or combine the similar methods
into 1 class ?

This is where the design principle DRY - don't repeat yourself would come in handy. Also, I am not a big fan of inheritance (I prefer composition), this would be a perfect candidate to build a abstract or must inherit class which should implement all the non database system specific code.

So a Access class will inherit this super class and will add it's own system specific part to the class. So e.g for Access or any OleDB related operation, the parameters of a parameterized query just use the ? as placeholder, but must ensure the order of the parameters.

General

You code seems to be too commented. For every setting/assignment you first write the comment what you are doing and then in the next line the code itself.

Some examples

// check if we should cache the results
if (_ShouldCache)  

// set the parameter data types
db.ParameterDataTypes = _ParamDTs;

This is blowing your codebase up to 1000 lines. Renaming your parameters and fields to something more meaningful would reduce the comments to the minimum.

Comments should be used to explain why something is done. What is done by the code should be documented by the code itself.

So by renaming _ShouldCache to shouldResultsBeCached (I don't like the underscores) and ParamDTs to parameterDataTypes you could remove the comments and Mr.Maintainer still would understand what your code does.

Access class

The // select which datatype, and force the official size part of the PrepareParams() method should be extracted to it's own method.

This tenary expression/operator

return (_PVs.Length > 0 && _PNs.Length > 0) ? true : false;

can be expressed like

return (_PVs.Length > 0 && _PNs.Length > 0);

which is more clear. This came form the method AreParams()

private bool AreParams() {
    // Check to see if the values and names are null first
    if (_PVs != null && _PNs != null) {
        try {
            Type _t_pv = _PVs.GetType();
            Type _t_pn = _PNs.GetType();
            if (_t_pv.IsArray && _t_pn.IsArray) {
                return (_PVs.Length > 0 && _PNs.Length > 0) ? true : false;
            } else {
                return false;
            }
        } catch {
            // yes I meant to do this, we really don't need to get the exception here
            return false;
        }
    } else {
        return false;
    }
}

If you use a guard condition, your code will be indented less. Adding up condition checks can remove if..else statements.

The method can be refactored and renamed to

private bool HasParameters() {
        // Check to see if the values and names are null first
        if (_PVs == null || _PNs == null) { return false;}

        try {
            Type _t_pv = _PVs.GetType();
            Type _t_pn = _PNs.GetType();
            return (_t_pv.IsArray && _t_pn.IsArray && _PVs.Length > 0 && _PNs.Length > 0);
            }
        } catch {
            // yes I meant to do this, we really don't need to get the exception here
            return false;
        }
    }

Using auto properties (where possible) can simplify your code also, as you name your properties meaningful, but not your backing fields.

Example

private CommandType _QT;
internal CommandType QueryType { set { _QT = value; } }

could become

internal CommandType QueryType { private get; set; }

Wrapper class

Inside your ExecuteWithReturn() method you are catching 2 different exception types, which you treat exactly the same (only the exception variable name differs). If you treat exceptions the same, you don't need to distinguish between them.

Style

Be consistent with the style you use. Try to use style the majority uses so a new developer will be used to it.

Example

Sometimes you put a opening brace { to a new line (where it belongs) sometimes at the end of the line.

StackExchange Code Review Q#68760, answer score: 11