2
\$\begingroup\$

I have a group of tests that must be run in similar (but different) fashion and they have setups, results, etc. which are also similar but different. Great, I thought, I'll use interfaces to define these tests and setups. However, I can't implement the setups and results as the specific types, the compiler requires that I must implement them as the root interface and cast them every time I access them, which feels wrong. How can I do this better?

This is probably better explained through code. Note that the property in ITest is an ISetup, because all tests will have setup properties and they will be different types:

Public Interface ITest
    Property Status As IStatus
    Property Setup As ISetup
    Property Result As IResult

    Sub StartTest()
    Sub RunTest()
    Sub PauseTest()
    Sub ResumeTest()
    Sub StopTest()
End Interface
Public Interface IStatus
    Property StartTime As DateTime
    Property SampleID As String
    Property Comments As String
End Interface
Public Interface ISetup
    Function IsValid() As Boolean
    Function GetValidityErrorMessage() As String
    Function ToString() As String
End Interface
Public Interface IResult
    Property PlotsToGraph As List(Of Plots)
    Property XData As List(Of Double())
    Property YData As List(Of Double())

    Property Status As IStatus
    Property Setup As ISetup
End Interface

and then I'll create an instance of these types. Here is the implementation for a Durability Test; others are similar but I can post them if you would like.

Namespace Global.Tester
    Public Enum ControlModes
        TableDisplacement
        SaddleDisplacement
        TableAcceleration
        SaddleAcceleration
    End Enum

    Public Enum Units
        mm
        g
    End Enum

    Public Enum Plots
        TableDisplacement
        SaddleDisplacement
        TableAcceleration
        SaddleAcceleration
        Transmissibility
        TableCommand
    End Enum
End Namespace

Public Class DurabilityResult
    Implements IResult

    Public Property PlotsToGraph As List(Of Plots) Implements IResult.PlotsToGraph
    Property XData As List(Of Double()) Implements IResult.XData
    Property YData As List(Of Double()) Implements IResult.YData

    Property Status As IStatus Implements IResult.Status
    Property Setup As ISetup Implements IResult.Setup

    Shared ReadOnly Property Extension As String
        Get
            Return ".dres"
        End Get
    End Property

    Public Sub New()
        Me.PlotsToGraph.AddRange({Plots.TableDisplacement, Plots.TableAcceleration, Plots.SaddleDisplacement, Plots.SaddleAcceleration})
        ' Add empty plots
        Me.XData.Add(Nothing)
        Me.XData.Add(Nothing)
        Me.XData.Add(Nothing)
        Me.XData.Add(Nothing)
        Me.XData.Add(Nothing)
        Me.XData.Add(Nothing)

        Me.YData.Add(Nothing)
        Me.YData.Add(Nothing)
        Me.YData.Add(Nothing)
        Me.YData.Add(Nothing)
        Me.YData.Add(Nothing)
        Me.YData.Add(Nothing)

        Me.Status = New DurabilityStatus
        Me.Setup = New DurabilitySetup
    End Sub
End Class
Public Class DurabilitySetup
    Implements ISetup
    Private logger As NLog.Logger = NLog.LogManager.GetCurrentClassLogger()

    Public Property ControlMode As _Tester.ControlModes

    Public Property SetPoint As Double
    Public Property Frequency As Double
    Public Property TotalCycles As Integer

    Public Property EnableSquirm As Boolean
    Public Property EnableLateral As Boolean

    <Xml.Serialization.XmlIgnore>
    Public Shared ReadOnly Property Extension As String
        Get
            Return ".jdtst"
        End Get
    End Property

    ' Current cycle, sample ID, start time, sample comments, etc. are not stored in a test document

    Public Function GetValidityErrorMessage() As String Implements ISetup.GetValidityErrorMessage
        If Me.SetPoint <= 0 Then
            Return "Set point must be greater than zero."
        End If
        If Me.Frequency <= 0 Then
            Return "Freqency must be greater than zero."
        End If
        If Me.TotalCycles <= 0 Then
            Return "Total cycles must be greater than zero."
        End If

        Return ""
    End Function

    Public Function IsValid() As Boolean Implements ISetup.IsValid
        Return GetValidityErrorMessage() = ""
    End Function

    Public Overrides Function ToString() As String Implements ISetup.ToString
        Dim sb As New System.Text.StringBuilder
        sb.Append("Control Mode: ")
        sb.AppendLine(Me.ControlMode.ToString())
        sb.Append("Set Point: ")
        sb.AppendLine(Me.SetPoint.ToString())
        sb.Append("Frequency: ")
        sb.AppendLine(Me.Frequency.ToString())
        sb.Append("Total Cycles: ")
        sb.AppendLine(Me.TotalCycles.ToString())
        Return sb.ToString()
    End Function

    <NUnit.Framework.TestFixture()> _
    Public Class TestDurabilityTest
        ' SetUp and TearDown run before each test
        <NUnit.Framework.SetUp()> _
        Public Sub SetUp()

        End Sub

        <NUnit.Framework.TearDown()> _
        Public Sub TearDown()

        End Sub

        <NUnit.Framework.Test()> _
        Public Sub TestDurabilityTest()
            Dim tst As New DurabilitySetup With {
                .ControlMode = _Tester.ControlModes.TableDisplacement,
                .Frequency = 2.33,
                .SetPoint = 25.4,
                .TotalCycles = 10000,
                .EnableSquirm = True,
                .EnableLateral = False
            }

            Console.WriteLine("DurabilityDocument initial configuration: ")
            Console.WriteLine(tst.ToString())
            Dim filename = IO.Path.Combine(Serializer.GetConfigurationDirectory, "Test DurabilityDocument Save and Load" + DurabilitySetup.Extension)
            Serializer.Save(filename, tst)
            Dim data As String = IO.File.ReadAllText(filename)
            Console.WriteLine("Saved XML")
            Console.WriteLine(data)
            Dim loadedTst = Serializer.Load(Of DurabilitySetup)(filename)
            Console.WriteLine("Loaded XML configuration: ")
            Console.WriteLine(loadedTst.ToString())
            Console.WriteLine("Validation: (blank means pass)")
            Console.WriteLine(loadedTst.GetValidityErrorMessage())

            NUnit.Framework.Assert.That(IO.File.Exists(filename))
            NUnit.Framework.Assert.That(loadedTst.IsValid())

            NUnit.Framework.Assert.That(loadedTst.ControlMode = Tester.ControlModes.TableDisplacement)
            NUnit.Framework.Assert.That(loadedTst.Frequency = 2.33)
            NUnit.Framework.Assert.That(loadedTst.SetPoint = 25.4)
            NUnit.Framework.Assert.That(loadedTst.TotalCycles = 10000)
            NUnit.Framework.Assert.That(loadedTst.EnableSquirm = True)
            NUnit.Framework.Assert.That(loadedTst.EnableLateral = False)
        End Sub
    End Class
End Class
Public Class DurabilityStatus
    Implements IStatus

    Public Property Comments As String Implements IStatus.Comments
    Public Property SampleID As String Implements IStatus.SampleID
    Public Property StartTime As Date Implements IStatus.StartTime

    Public Property CurrentCycle As Integer
    Public Property TableDisplacement As Double
    Public Property TableAcceleration As Double
    Public Property SaddleDisplacement As Double
    Public Property SaddleAcceleration As Double
End Class

Note the use of properties which are not in the interface like DurabilitySetup.SetPoint. The test is currently in the following state; I'm working to make it complete but got side-tracked when I realized that all of the above interfaces would need to be declared as their interface type instead of their instance.

Public Class JounceDurabilityTest
    Implements ITest
    Private logger As NLog.Logger = NLog.LogManager.GetCurrentClassLogger()

    Property Status As IStatus Implements ITest.Status
    Property Setup As ISetup Implements ITest.Setup
    Property Result As IResult Implements ITest.Result

    Private Enabled As Boolean
    Private TestState As TestStates = TestStates.NotRunning
    Private Enum TestStates
        Starting
        Resuming
        Pausing
        Stopping
        Running
        NotRunning
    End Enum

    Private SineGenerator As New SineGenerator(0, 1, 1 / Globals.NI.SampleRate, Globals.NI.SampleSize)
    Private timeInStep As Double = 0

    Sub StartTest() Implements ITest.StartTest
        Me.Enabled = True
        Me.TestState = TestStates.Starting
    End Sub

    ' Called every 20ms by hardware interrupt
    Sub RunTest() Implements ITest.RunTest
        Dim setup As DurabilitySetup = DirectCast(Me.Setup, DurabilitySetup)

        If Not Me.Enabled Then
            ' Do nothing
            Exit Sub
        End If
        Select Case Me.TestState
            Case TestStates.Starting
                If timeInStep = 0 Then
                    SineGenerator = New SineGenerator(0, setup.Frequency, 1 / Globals.NI.SampleRate, Globals.NI.SampleSize)
                ElseIf SineGenerator.Amplitude < setup.SetPoint Then
                    SineGenerator.SetNextAmplitude(SineGenerator.Amplitude + setup.SetPoint / 100)
                End If

                Dim nextValues = SineGenerator.Next()
                For i As Integer = 0 To Globals.NI.SampleSize - 1
                    Globals.NI.AnalogOutputs(AnalogOutputEnum.PositionOutput).Values.Enqueue(nextValues(i))
                Next
            Case TestStates.Resuming
            Case TestStates.Pausing
            Case TestStates.Stopping
            Case TestStates.NotRunning
        End Select

        timeInStep += Globals.NI.SampleSize / Globals.NI.SampleRate
    End Sub
    Sub PauseTest() Implements ITest.PauseTest

    End Sub
    Sub ResumeTest() Implements ITest.ResumeTest

    End Sub
    Sub StopTest() Implements ITest.StopTest

    End Sub
End Class

I would like to do the following instead:

    Property Setup as DurabilitySetup Implements ITest.Setup
    Public Sub RunTest() Implements ITest.RunTest
        SineGenerator.SetNextAmplitude(Setup.SetPoint)

This small bit of code may not seem problematic, but it is pervasive and annoying when you have ISetup, IResult, IStatus, etc, many methods which access them, and many ITest implementations which all go through this same hassle.

Is there a better way to do this?

\$\endgroup\$
  • 1
    \$\begingroup\$ OK, I'm more familiar with Stack Overflow, so I posted abbreviated code. It's still a work in progress; I got slowed down by this issue but just give me a few minutes and I'll try to take care of it. From looking at other questions and the help, I understand that you want all of the relevant classes, but not the full UI and everything else? \$\endgroup\$ – kvermeer Oct 29 '14 at 13:13
  • 1
    \$\begingroup\$ Yes, please include only the working portion of code you would like reviewed. \$\endgroup\$ – xDaevax Oct 29 '14 at 13:21
  • 1
    \$\begingroup\$ There it is! There are other classes referenced in the code (Serializer, SineGenerator, NI, AnalogOutput...) but those aren't really relevant to the design of the above interfaces, and they in turn refer to other classes. Would you like me to post them anyways? \$\endgroup\$ – kvermeer Oct 29 '14 at 13:38
  • \$\begingroup\$ Looks sufficient to me @kvermeer. Thanks. Retracting my downvote now. =) Welcome to Code Review! \$\endgroup\$ – RubberDuck Oct 29 '14 at 14:18
1
\$\begingroup\$

Interface

An interface is a contract which ensures that an object which implements this interface also implements/has the defined methods and properties.
Not more not less.

So to avoid the cast each time the RunTest() method is called, you can cast it once and store the casted value in the desired field.

Like

Private theCastedSetup As DurabilitySetup
Public ReadOnly Property CastedSetup As DurabilitySetup
    Get
        If (theCastedSetup Is Nothing) Then
            theCastedSetup = DirectCast(Me.Setup, DurabilitySetup)
        End If
        Return theCastedSetup
    End Get
End Property  

So, if you need the DurabilitySetup you use it like

SineGenerator = New SineGenerator(0, Me.CastedSetup.Frequency, 1 / Globals.NI.SampleRate, Globals.NI.SampleSize)

and if you need the setup as ISetup, you just pass the Me.Setup.

JounceDurabilityTest

Private SineGenerator As New SineGenerator(0, 1, 1 / Globals.NI.SampleRate, Globals.NI.SampleSize)  

should be replace by

Private SineGenerator As SineGenerator  

as there is no need to create a SineGenerator object here.

DurabilitySetup

If you change the overridden ToString() method to

Public Overrides Function ToString() As String Implements ISetup.ToString
    Dim sb As New System.Text.StringBuilder
    sb.Append("Control Mode: ").AppendLine(Me.ControlMode.ToString())
    sb.Append("Set Point: ").AppendLine(Me.SetPoint.ToString())
    sb.Append("Frequency: ").AppendLine(Me.Frequency.ToString())
    sb.Append("Total Cycles: ").AppendLine(Me.TotalCycles.ToString())
    Return sb.ToString()
End Function

one can better see what belongs to what.

Instead of checking GetValidityErrorMessage() = "" you can use String.IsNullOrWhiteSpace(GetValidityErrorMessage()).

But you still could do better.

Let us introduce some more methods.

Private Function IsPointValid() as Boolean
    Return Me.SetPoint > 0
End Function

Private Function IsFrequencyValid() as Boolean
    Return Me.Frequency > 0
End Function

Private Function IsTotalCyclesValid() as Boolean
    Return Me.TotalCycles > 0
End Function

Now we can refactor the IsValid() method to

Public Function IsValid() As Boolean Implements ISetup.IsValid
    Return IsPointValid() AndAlso IsFrequencyValid() AndAlso IsTotalCyclesValid()
End Function

and the GetValidityErrorMessage() method to

Public Function GetValidityErrorMessage() As String Implements ISetup.GetValidityErrorMessage
    If Not IsPointValid() Then
        Return "Set point must be greater than zero."
    End If
    If Not IsFrequencyValid() Then
        Return "Freqency must be greater than zero."
    End If
    If Not IsTotalCyclesValid() Then
        Return "Total cycles must be greater than zero."
    End If

    Return String.Empty
End Function

General

You should better use String.Empty over "" as it is more readable.

\$\endgroup\$
  • \$\begingroup\$ Wow, thanks for all the great suggestions. I guess that CastedSetup will work, I was just hoping that there was a better way to create the relationships. What is the reason for using the "If IsNothing create private variable / return privateVariable" sequence for CastedSetup instead of just "return DirectCast(Setup, DurabilityTest"? \$\endgroup\$ – kvermeer Oct 29 '14 at 15:09
  • \$\begingroup\$ If the casted one is needed more than once, you don't have to cast it again. \$\endgroup\$ – Heslacher Oct 29 '14 at 15:13
  • \$\begingroup\$ I get that, but I didn't think that casting was an expensive operation - especially because it's a DirectCast, which should just return a reference to the object with the correct type? \$\endgroup\$ – kvermeer Oct 29 '14 at 15:22
  • \$\begingroup\$ Also, it's important that the property behaves in an access-by-reference format. For example, the Status must be propagated through such that when RunTest() changes CastedStatus.CurrentCycle, external code calling Test.Status.ToString() where test is an ITest gets a reference to the class that has the updated cycle count. Does that still work with a one-time cast? \$\endgroup\$ – kvermeer Oct 29 '14 at 15:25
  • \$\begingroup\$ You could write a test to test this ;-) I don't see any problem with the ToString with a one time cast, as the underlaying object won't change. If you set the Setup property to a different Setup object you will for sure get problems. But you could set theCastedSetup = Nothing inside the Setup setter. \$\endgroup\$ – Heslacher Oct 29 '14 at 15:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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