Database Class Creator

Overview

I've written a class that will create C# code. The output is a single .cs file for each table in the default database (the database is defined inside a web.config file). I'm looking for a code review on this class alone, not on the generated code. This DatabaseClassCreator class uses the DatabaseAccess class for some of its database access. The DatabaseAccess class can be seen here as I'm asking for a code review of that class as well. If you are not interested in seeing the DatabaseAccess class, there is one static method ExecSQL, which returns a single DataTable with the results of the passed SQL.

Two notes:

• This was developed and is being used for a ASP.NET project, which is the reason for the BulletedList.

• This class has to work in a C# .NET 2 environment, so anything that's more modern would be interesting to me, but please note in your answer if the comments/feedback require a newer .NET version.

```
///
/// This class will create c# class files to access spesific
/// tables in a database.
///
public static class DatabaseClassCreator
{
///
/// Create class files for all the non-system tables in the current
/// default database.
///
/// The output location for the class files. This
/// is a fully qualified path.
public static void CreateAllTables(string OutputPath)
{
BulletedList bl = new BulletedList();
CreateAllTables(bl, OutputPath);
}
///
/// Create class files for all the non-system tables in the current
/// default database.
///
/// A BulletedList where status information can be
/// added to.
/// The output location for the class files. This
/// is a fully qualified path.
public static void CreateAllTables(BulletedList StatusBulletList, string OutputPath)
{
DataTable table = ListDatabaseTables();

if (table == null)
{
ListItem liRowName = new ListItem();
liRowName.Text = "Database

1) As already mentioned T4 might be a better solution though I'm not sure whether it is available for earlier VS versions.

2) I would avoid using string OutputPath parameters, Streams are usually more handy.

3) Separate presentation and business logic code. Having BulletedList parameter in such a class seems to be completely wrong to me.

4) I think you should not even take this BulletedList (or anything similar) as parameter. This messages log is an output of your methods, not an input, so there is no point accepting it as parameter. Return it and remove method overloads which are not needed.

5) Naming convention - usual rules are lower camel case for parameters.

6) Define variables closer to their first assignment and do not assign them any value if this value will be overwritten anyway. This is mostly about CreateSingleTable method. ListDatabaseTables also has 5 lines of code instead of maximum 3 needed.

7) I would make ListDatabaseTables() method more strongly typed. Returning a DataTable gives me absolutely no idea how to use it.

8) I prefer writing such constructions:

if (f.readOnly)
                info += " (RO)";
            else
                info += " (RW)";

as:

info += f.readOnly ? " (RO)" : " (RW)";

This IMO shows more clearly that you're going to append something to info anyway and this something depends on isReadonly value.

9) I would replace this:

string info = "---> " + f.name + " (" + f.type + ")";
if (f.readOnly)
    info += " (RO)";
else
    info += " (RW)";
if (f.allowNull)
    info += " (Null)";

with this:

string info = string.Format("---> {0} ({1}) ({2}){3}"
                                 , f.name
                                 , f.type
                                 , f.readOnly? "RO" : "RW"
                                 , f.allowNull ? " (NULL)" : string.Empty);

This shows more clearly which format will info variable have. Also this has only one assignment which is better than doing += on string several times.

10) sw.WriteLine("// Inherit this class and make changes there"); I would prefer extending existing class instead of inheritance. Or at least you should allow extending it. In order to allow this generated classes are usually defined as partial.

11) A lot of repeats here:

if (getType(f.type) == "int")
            sw.WriteLine("\tprivate " + getType(f.type) + " _" + f.name + " = -1;");
        else if (getType(f.type) == "float")
            sw.WriteLine("\tprivate " + getType(f.type) + " _" + f.name + " = -1;");
        else if (getType(f.type) == "DateTime")
            sw.WriteLine("\tprivate " + getType(f.type) + " _" + f.name + " = new DateTime(1753, 1, 1);");
        else if (getType(f.type) == "byte[]")
            sw.WriteLine("\tprivate " + getType(f.type) + " _" + f.name + " = new byte[1];");
        else
            sw.WriteLine("\tprivate " + getType(f.type) + " _" + f.name + ";");

It should be separated into at least two blocks:

a) write private + type

b) determine default value and write it (if any)

12) You have new DateTime(1753, 1, 1); repeated several times in your code. I would consider this string as magic string and I think it should be extracted into constant.

13) fieldName.ToUpper() == "DEFAULT". string.Equals(...) has a parameter to ignore case.

14) You have just written your own string.Join(...) here:

count = fields.Count;
    foreach (TableFieldInfo f in fields)
    {
        count--;
        if (f.readOnly != true)
        {
            if (count != 0)
                sw.Write("@" + f.name + ", ");
            else
                sw.Write("@" + f.name);
        }
    }

15) if (f.readOnly == true && CanSelect == false) I do not think this is the case when bool variable should be compared against true or false. I would prefer if (f.readOnly && !CanSelect)

16) Do not write god methods/classes and do not instantiate god objects. CreateSingleTable is definitely a god method - it has almost 500 lines of code !!! Raptors will come for you as soon as they will finish with goto writers. Break down this method into ~5-10 smaller methods.

17)

```
private static string getType(string DBType)
{
string ret = DBType;

if (DBType == "System.Data.SqlTypes.SqlString")
ret = "string";
else if (DBType == "System.Data.SqlTypes.SqlInt16")
ret = "Int16";
else if (DBType == "System.Data.SqlTypes.SqlInt32")
ret = "int";
else if (DBType == "System.Data.SqlTypes.SqlFloat")
ret = "float";
else if (DBType == "System.Data.SqlTypes.SqlDouble")
ret = "float";
else if (DBType == "System.Data.SqlTypes.SqlDecimal")
ret = "float";
else if (DBType == "System.Data.SqlTypes.SqlBoolean")
ret = "bool";
else if (DBType == "System.Data.SqlTypes.SqlDateTime")
ret = "DateTime";
else if (DBType == "System.Data.S

StackExchange Code Review Q#2595, answer score: 12