Generating controls based on PropertyInfo

This is a class that takes a PropertyInfo and makes a Control for it. You can access the value of the control through a property on the class called Value. Basically I get the data from the database for a specific type and create a form where I add the controls generated by instances of this class for each property of the type. Then, I can get the values back, fill the entity with them and save the changes to the database.

```
Imports System.Reflection
Imports System.Data.Entity
Imports System.Data.Entity.Core.Common.EntitySql
Imports System.Runtime.Remoting.Messaging
Imports System.IO
Imports System.Linq.Expressions
Imports SCD.Forms
Imports SCD.Helpers
Imports Microsoft.VisualBasic.ApplicationServices
Imports SCD.Models
Imports SCD.Controls
Imports SCD.Services

Namespace AppModels
Public Interface IViewModelControlMapping
End Interface

Public Class ViewModelControlMapping
Implements IViewModelControlMapping
Public Property PropertyInfo As PropertyInfo
Public Property Control As Control
Private Function GetControl(Of TC As Control)() As TC
Return DirectCast(Control, TC)
End Function
Public Property Value As Object
Get
Return GetterFunc()
End Get
Set(_value As Object)
If (SetterFunc IsNot Nothing) Then SetterFunc(_value)
End Set
End Property
Private GetterFunc As Func(Of Object)
Private SetterFunc As Func(Of Object, Object)

Public Sub New(propertyInfo As PropertyInfo)
Me.PropertyInfo = propertyInfo
If (Not propertyInfo.GetMethod.IsVirtual) Then
Select Case propertyInfo.PropertyType.Name
Case "Byte[]"

Case Else
Control = New LabelTextBox
Control.Dock = DockStyle.Top
GetControl(Of LabelTextBox).Label.Text = S(Me.PropertyInfo.Name)

Public Interface IViewModelControlMapping
End Interface

This is called a marker interface - an interface that does nothing. It's a design smell more often than not, because it's misusing interfaces to convey metadata. The .net framework has attributes for that; when you need metadata for a type, it's best to decorate it with an attribute.

Granted, it's much easier to verify whether a type implements a marker interface, than to use reflection to verify whether it's decorated with such or such attribute... but I'm not going to extrapolate about whether or not you're using the interface as metadata. Instead, I'm going to suggest making it useful!

Property PropertyInfo As PropertyInfo
Property Control As Control
Property Value As Object

The interface should have at least these members; they're public members of any ViewModelControlMapping object, and they're part of that object's interface1 anyway.

Now, where's the code that's doing all the work?

Public Sub New(propertyInfo As PropertyInfo)

The constructor! The constructor is doing all the work! This can't be right. You have quite a lot of logic in there; I'd start refactoring by extracting private methods out of each If/Else block.

The GetControl generic method is interesting:

Private Function GetControl(Of TC As Control)() As TC
    Return DirectCast(Control, TC)
End Function

I'm not too fond of the VB.NET syntax, so for my own sake I'll "translate" it:

private TC GetControl() where TC : Control
{
    return (TC)Control;
}

Okay. I hope I got this right, the VB syntax for generics is quite foreign to my see-sharp eyes.. Point is, I don't think that method is really needed; all it does is substitute the DirectCast keyword (statement?) for a just-as-convoluted GetControl generic method call: the code would be more self-explanatory if the DirectCast remained at the call sites, so another refactoring I'd do, is inline all these method calls and remove GetControl.

Select Case propertyInfo.PropertyType.Name
    Case "Byte[]"

    Case Else
        Control = New LabelTextBox
        Control.Dock = DockStyle.Top
        GetControl(Of LabelTextBox).Label.Text = S(Me.PropertyInfo.Name)
        GetterFunc = Function() As Object
                         Return GetControl(Of LabelTextBox).TextBox.Text
                     End Function
        SetterFunc = Function(o As Object)
                         GetControl(Of LabelTextBox).TextBox.Text = o
                         Return True
                     End Function
End Select

Why a Select Case with nothing but an Else branch? This really looks like it should be an If block that does nothing if the type is a Byte[] - the question is, why special-case a byte array, and not a Smurf[]?

You're calling GetControl (and so, casting that Control object) 3 times in a row here. Why not declare a LabelTextBox local variable, and code against that instead? If closures work the same way in VB as they do in C#, there shouldn't be a problem with doing that.

I have a bit of a problem with a "Setter" function. Setters don't return true. Setters just set a value, and don't return anything: a setter isn't a Function, it's an Action - an Action delegate that takes one parameter. Don't ask me the VB syntax for it though.

This looks redundant:

If (o IsNot Nothing) Then
     GetControl(Of CrudICollectionControl).Chips = o
 Else
     GetControl(Of CrudICollectionControl).Chips = Nothing
 End If

If o isn't Nothing, you assign o. Otherwise, you assign Nothing. Why not just assign o and thus, have it be Nothing when o is Nothing?

It looks like the architecture is wrong. There is a different type involved in each conditional branch, and I don't recognize any of them - either my WinForms is much more outdated than I thought, or each branch creates user-defined custom UserControl objects - and with time the list will only grow, and that constructor will get more and more bloated and convoluted.

I don't have an immediate solution for it, but - food for thought - I'd consider implementing a Strategy Pattern here.

1 an object's interface is the public members it exposes - its API, in strict OOP terms; not to be confused with an Interface, which is a type.

StackExchange Code Review Q#101693, answer score: 6