current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

In my application, we have a number of pages which provide an advanced search capability, with a large number of potential search options (some pages have as few as 3, some as many as 150).

The HTML for the page is generated with a standard .aspx page, and the search criteria themselves are stored in a DTO which is then converted to a namevaluecollection and encoded into the query string, which is passed to the results page and then decoded back into a DTO (which is then further transformed into a paramarray and passed to the DAL to retrieve the results). This is all fine so far.

However, there is a new requirement that if a DTO is provided to the search criteria page (eg through the querystring) that the criteria be automatically filled out. The basic way of implementing this is to manually check each item in the DTO and appropriately update the criteria controls. This has the downside that it means that there are two mappings which need to be maintained, but are identical (though mirrored); DTO Property->Control (page load), Control->DTO Property (search button clicked).

Instead, I'd like to make a one time mapping between the control and the DTO properties. This is the method I'm using, I'd like to know peoples thoughts on it in terms of complexity, maintainability, implementation specifics. Is this a good idea? Am I doing it correctly?

Note that the interfaces are there to allow you to bind custom server controls by exposing their properties.

''' <summary>The base page that all searches inherit from.  Contains functionality for loading searches from saved searches.</summary>
Public MustInherit Class BaseSearchPage(Of Tdto As {New, baseSearchDTO})
    Inherits System.Web.UI.Page

    ''' <summary>Provides a binding of a control to one or more properties in a DTO.</summary>
    Private Class ControlBinding
        ''' <summary>A reference to the control which will be bound.</summary>
        Public Property Control As Control
        ''' <summary>The property(ies) which the control will be bound to.</summary>
        Public Property Props As New List(Of PropertyInfo)

        Public Sub New(ctrl As Control, ParamArray props() As PropertyInfo)
            Me.Control = ctrl
            Me.Props.AddRange(props)
        End Sub
    End Class

    ''' <summary>A list of all the control to dto bindings for this page.</summary>
    Private Property ControlBindings As New List(Of ControlBinding)

    ''' <summary>The parameters of the search which will be used for redirecting.</summary>
    Protected Property SearchParams As Tdto

    ''' <summary>Override this sub to bind controls to their properties on the dto.</summary>
    Protected MustOverride Sub BindControlsToProperties()

    ''' <summary>Binds a control to a single property of the DTO.
    ''' Usage: BindControlToProperty(trvIndustries, Function () Me.SearchParams.IndustryGroups, Function() Me.SearchParams.Industries, Function() Me.SearchParams.IndustrySectors) ... </summary>
    Protected Overloads Sub BindControlToProperty(Of T)(ctrl As Control, ParamArray linqExpressions() As System.Linq.Expressions.Expression(Of Func(Of T)))
        Dim props As New List(Of PropertyInfo)

        For Each linqExpression In linqExpressions
            Dim memberEx As MemberExpression = linqExpression.Body
            Dim prop As System.Reflection.PropertyInfo = memberEx.Member

            props.Add(prop)
        Next

        ControlBindings.Add(New ControlBinding(ctrl, props.ToArray))
    End Sub

    ''' <summary>Using the control to property bindings, set the controls to their correct state.</summary>
    Protected Sub SelectFromDTOs()
        ' Loop through the control bindings and select the appropriate items in the control based on the control type.
        For Each ctrlBinding As ControlBinding In Me.ControlBindings

            If GetType(ListControl).IsAssignableFrom(ctrlBinding.Control.GetType) Then
                ' List controls ...
                If ctrlBinding.Props(0).PropertyType = GetType(Integer) Or
                    ctrlBinding.Props(0).PropertyType = GetType(Nullable(Of Integer)) Or
                    ctrlBinding.Props(0).PropertyType.IsEnum Or
                    ctrlBinding.Props(0).PropertyType.IsNullableEnum Then
                    ' List controls bound to an int / enum / nullable(int/enum) - wraps the output into an array
                    CType(ctrlBinding.Control, ListControl).SelectFromIntList({ctrlBinding.Props(0).GetValue(Me.SearchParams)})
                Else
                    ' List controls bound to a List(of Int)
                    CType(ctrlBinding.Control, ListControl).SelectFromIntList(ctrlBinding.Props(0).GetValue(Me.SearchParams))
                End If

            ElseIf GetType(CheckBox).IsAssignableFrom(ctrlBinding.Control.GetType) Then
                ' Checkboxes ...
                CType(ctrlBinding.Control, CheckBox).Checked = ctrlBinding.Props(0).GetValue(Me.SearchParams)

            ElseIf GetType(Interfaces.IPropertyBindableControl).IsAssignableFrom(ctrlBinding.Control.GetType) Then
                ' Code for interface objects ...

            Else
                Throw New ArgumentOutOfRangeException("BaseSearchPage.SelectFromDTOs exception: control [" & ctrlBinding.Control.ID & "] is of unhandled control type [" & ctrlBinding.Control.GetType.ToString & "].")
            End If
        Next
    End Sub
End Class

Namespace Interfaces
    ''' <summary>The base interface for IPropertyBindableControls ... you should not implement this.</summary>
    Public Interface IPropertyBindableControl
    End Interface

    ''' <summary>Defines a custom/server control which can have one of its properties bound to a DTO.</summary>
    ''' <typeparam name="T1">The type of the first property.</typeparam>
    Public Interface IPropertyBindableControl(Of T1)
        Inherits IPropertyBindableControl

        Property Prop1 As T1
    End Interface

    ''' <summary>Defines a custom/server control which can have two of its properties bound to a DTO.</summary>
    ''' <typeparam name="T1">The type of the first property.</typeparam>
    ''' <typeparam name="T2">The type of the second property.</typeparam>
    Public Interface IPropertyBindableControl(Of T1, T2)
        Inherits IPropertyBindableControl

        Property Prop1 As T1
        Property Prop2 As T2
    End Interface

    ''' <summary>Defines a custom/server control which can have three of its properties bound to a DTO.</summary>
    ''' <typeparam name="T1">The type of the first property.</typeparam>
    ''' <typeparam name="T2">The type of the second property.</typeparam>
    ''' <typeparam name="T3">The type of the third property.</typeparam>
    Public Interface IPropertyBindableControl(Of T1, T2, T3)
        Inherits IPropertyBindableControl

        Property Prop1 As T1
        Property Prop2 As T2
        Property Prop3 As T3
    End Interface

End Namespace

And on the .aspx page itself:

Protected Overrides Sub BindControlsToProperties()
    BindControlToProperty(rblSearchBy, Function() SearchParams.SearchType)
    BindControlToProperty(cbIncludeFundsCurrentlyRaising, Function() SearchParams.IncludeFundsCurrentlyRaising)
    etc ...
share|improve this question
    
Of particular interest to me is whether this is the "correct" way to maintain a binding between controls and dto properties. –  mrmillsy Nov 6 '14 at 10:02

1 Answer 1

up vote 1 down vote

private class ControlBinding

As you are using Me.Props.AddRange(props) inside the constructor, you are expecting an IEnumerable(Of PropertyInfo) as the second input parameter.
You should change constructors signature to reflect this. 

Public Sub New(ctrl As Control, props As IEnumerable(Of PropertyInfo))
    Me.Control = ctrl
    Me.Props.AddRange(props)
End Sub

class BaseSearchPage

You should always try to code against an interface instead of against an implementation. As you don't need any methods of the List(Of T) class, you should change the BindControlToProperty() method to 

Protected Overloads Sub BindControlToProperty(Of T)(ctrl As Control, ParamArray linqExpressions() As System.Linq.Expressions.Expression(Of Func(Of T)))
    Dim props As IList(Of PropertyInfo) = New List(Of PropertyInfo)()

    For Each linqExpression In linqExpressions
        Dim memberEx As MemberExpression = linqExpression.Body
        Dim prop As System.Reflection.PropertyInfo = memberEx.Member

        props.Add(prop)
    Next

    ControlBindings.Add(New ControlBinding(ctrl, props))
End Sub

As you see, because we have changed the constructors argument of the ControlBinding class, we can omit the call to props.ToArray().

In the SelectFromDTOs() method you should use the short circuit boolean operator OrElse instead of Or.

So 

If ctrlBinding.Props(0).PropertyType = GetType(Integer) Or
    ctrlBinding.Props(0).PropertyType = GetType(Nullable(Of Integer)) Or
    ctrlBinding.Props(0).PropertyType.IsEnum Or
    ctrlBinding.Props(0).PropertyType.IsNullableEnum Then

should become 

If ctrlBinding.Props(0).PropertyType = GetType(Integer) OrElse
    ctrlBinding.Props(0).PropertyType = GetType(Nullable(Of Integer)) OrElse
    ctrlBinding.Props(0).PropertyType.IsEnum OrElse
    ctrlBinding.Props(0).PropertyType.IsNullableEnum Then

If you use the Or operator then each condition will be evaluated, no matter if it is evaluated to trueor not.
If using the OrElse operator the following expressions won't be evaluated if the current evaluates to true.

share|improve this answer
    
Thanks for the feedback, I've included your suggestions. –  mrmillsy Nov 5 '14 at 15:27

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.