Database Dynamic Table Creation

Question

The code below is meant to build an array with certain properties neccesairy to create column for a table in a database.
Would there be any more effcient way of doing this?

Public fieldName As String
Public fieldType As ADOX.DataTypeEnum
Public fieldProperties As String
Public fieldPropertiesValue
Public fieldIndex As Boolean
Public fieldUnique As Boolean
Public fieldKey As Boolean

Public Sub New(ByVal fieldName As String, ByVal fieldType As ADOX.DataTypeEnum, Optional ByVal fieldProperties As String = Nothing,
               Optional ByVal fieldPropertiesValue As Object = Nothing, Optional ByVal fieldIndex As Boolean = False,
               Optional ByVal fieldUnique As Boolean = False, Optional ByVal fieldKey As Boolean = False)

    Me.fieldName = fieldName
    Me.fieldType = fieldType
    Me.fieldProperties = fieldProperties
    Me.fieldPropertiesValue = fieldPropertiesValue
    Me.fieldIndex = fieldIndex
    Me.fieldUnique = fieldUnique
    Me.fieldKey = fieldKey

End Sub

Public Shared IDOwner As classObjectField = New classObjectField("IDOwner", ADOX.DataTypeEnum.adInteger, "AutoIncrement", True, True, True, True)
Public Shared OwnerAccount As classObjectField = New classObjectField("OwnerAccount", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)
Public Shared OwnerName As classObjectField = New classObjectField("OwnerName", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)
Public Shared OwnerMail As classObjectField = New classObjectField("OwnerMail", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True, fieldUnique:=True)

Public Shared ArrayTableOwners = New Object(3) {
    IDOwner, OwnerAccount, OwnerName, OwnerMail
}

Public Shared IDUser As classObjectField = New classObjectField("IDUser", ADOX.DataTypeEnum.adInteger, "AutoIncrement", True, True, True, True)
Public Shared UserName As classObjectField = New classObjectField("OwnerAccount", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)
Public Shared UserFunctionCode As classObjectField = New classObjectField("OwnerName", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)

Public Shared ArrayTableUser = New Object(2) {
    IDUser, UserName, UserFunctionCode
}

This function below creates the table of an existing Access DB

 Public Shared Function createDBTable(Cat As ADOX.Catalog, tableName As String, objField As Object) As Boolean
    Dim confirm As Boolean = True

    Try
        Dim objTable = New ADOX.Table
        Dim objIndex = New ADOX.Index

        objTable.Name = tableName

        For Each valueField In objField

            objTable.Columns.Append(valueField.fieldName, valueField.fieldType)

            If Not valueField.FieldProperties = Nothing Then

                objTable.Columns.Item(valueField.fieldName).ParentCatalog = Cat
                objTable.Columns.Item(valueField.fieldName).Properties(valueField.fieldProperties).Value = valueField.FieldPropertiesValue

            End If

            If valueField.fieldIndex = True Then

                objIndex = New ADOX.Index
                objIndex.Unique = valueField.fieldUnique
                objIndex.Name = valueField.fieldName
                objIndex.PrimaryKey = valueField.fieldKey
                objIndex.Columns.Append(valueField.fieldName)
                objTable.Indexes.Append(objIndex)

            End If

        Next

        Cat.Tables.Append(objTable)

        objIndex = Nothing
        objTable = Nothing
    Catch ex As Exception

        MsgBox(ex.Message)
        confirm = False

    End Try


    Return confirm
End Function

Usage:

Dim objCn = New ADODB.Connection
Dim objCat = New ADOX.Catalog
objCn.Open(DBSourceLocation)
objCat.ActiveConnection = objCn

createDBTable(objCat, "OWNERLIST", ArrayTableOwners)
createDBTable(objCat, "USERLIST", ArrayTableUser)

Answer 1

Encapsulation is taking a beating here:

Public fieldName As String
Public fieldType As ADOX.DataTypeEnum
Public fieldProperties As String
Public fieldPropertiesValue
Public fieldIndex As Boolean
Public fieldUnique As Boolean
Public fieldKey As Boolean

You have a public constructor for assigning these fields, but the instance fields are all Public, which completely defeats the purpose of encapsulation. Fields should be Private, and ideally read-only (I'm a C# guy, not sure how/whether VB.NET implements this but in C# that would be private readonly [type] [name];).

It gets worse:

Public Shared IDOwner As classObjectField = New classObjectField("IDOwner", ADOX.DataTypeEnum.adInteger, "AutoIncrement", True, True, True, True)
Public Shared OwnerAccount As classObjectField = New classObjectField("OwnerAccount", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)
Public Shared OwnerName As classObjectField = New classObjectField("OwnerName", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)
Public Shared OwnerMail As classObjectField = New classObjectField("OwnerMail", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True, fieldUnique:=True)

Beyond the constructor [made redundant by public fields], nothing in this class seems to justify having an instance: literally everything is Shared! These public shared object fields are statically initialized but, being fields, any calling code can swap the reference at any time, and because the fields are Shared, every single instance out there suddenly finds itself with a modified IDOwner.

Expose properties, not fields; and only expose Property Get if outside/calling code shouldn't be allowed to tamper with the encapsulated values and references.

OOP is about objects. Shared makes a member belong to a type, not an object instance: Shared is essentially the complete opposite of Object-Oriented.

Seems you mean to use the Public Shared fields as a way to return "canned" instances of this classObjectField class - that's not an awful idea, but then the constructor should be Private.

classObjectField is a bad name for a class. "class" is perfectly redundant and has nothing to do in the name of the type, and class and member names should be PascalCase, so that would be ClassObjectField, and CreateDbTable.

---

VB.NET isn't just Object-Oriented: it's also *strongly-typed.

Public Shared ArrayTableOwners = New Object(3) {
    IDOwner, OwnerAccount, OwnerName, OwnerMail
}

This is an array of Object references; in .NET everything is an Object - from a simple String to any class instance. Exposing Object on your public API is considered bad practice, because the caller has no idea what they're getting. This would be better (failed encapsulation aside):

Public Shared ArrayTableOwners = New classObjectField(3) {
    IDOwner, OwnerAccount, OwnerName, OwnerMail
}

Now the caller knows they're getting an array of classObjectField objects.

Public Shared Function createDBTable(Cat As ADOX.Catalog, tableName As String, objField As Object) As Boolean

I don't see a reason not to rename Cat to adoxCatalog - it's a catalog, not a cat. And objField is terribly misleading.

The signature says objField As Object, implying ONE object is expected. But then...

    For Each valueField In objField

If you expect an array, make the signature say so! This is what I meant with an object can be literally anything. And for performance reasons you should iterate arrays with a For loop, not For Each.

Actually, keep the For Each. Because you don't need an array here - you need anything that can be iterated. The .NET framework gives you the generic IEnumerable(Of T) interface for that: objField should be plural, to indicate it can "contain" many objects, and then instead of As Object it should be As IEnumerable(Of classObjectField), so that you can't give it a String and still happily compile and run the project.

The function shouldn't return a Boolean. Even your sample usage code doesn't care about the return value:

createDBTable(objCat, "OWNERLIST", ArrayTableOwners)
createDBTable(objCat, "USERLIST", ArrayTableUser)

Truth is, nobody will either, for you've already handled the exception and displayed a message box - and that's not your job!

Instead, make the function a Sub, and let exceptions bubble up the call stack, so that the calling code is responsible for handling exceptions and deciding whether they want to display a message box or log an entry to some text file or whatever. .NET works with exceptions, not return codes.

That said if I had to return a Boolean value for success, I'd default it to False and only switch it to True when I'm absolutely certain that my function did what it had to do. Here you're defaulting to True and switching it to False when an exception is thrown, which only works because you're catching all exceptions - but best practices tell us that we should only be catching the most specific exceptions we can, and let everything else bubble up - and if you did that, your function could return True when it really actually failed. confirm sounds like some flag that would be True when the user confirmed some prompt; success would be a better name for it. But that's moot anyway, as I said it's not that function's job to handle all possible exceptions and display a message box.