[RESOLVED] How to write this more compact?

[MrNorth]

Hi!

I have done mostly C# and now I am programming VB.NET and I have a few questions on how to optimize my code:

First

 If performanceTest.TestResultUnits = 4 Then
            performanceTest.TestResultUnits = 12
        End If

        If performanceTest.FlowScaleUnits = 4 Then
            performanceTest.FlowScaleUnits = 12
        End If

        If performanceTest.HeadScaleUnits = 4 Then
            performanceTest.HeadScaleUnits = 12
        End If

        If performanceTest.PowerScaleUnits = 4 Then
            performanceTest.PowerScaleUnits = 12
        End If

This feels kind of bloated, is there better syntax to handle this? The scenario is in the repository where an enum is converted to a number which is the type expected in the db, and sadly the db is not based on binary representation so I have to manually convert the value before saving the performanceTest object with Entity Framework

Next

 Private Sub SetHeaders(selectedUnit As DisplayUnits)
        Dim flow As String = String.Empty
        Dim head As String = String.Empty
        Dim power As String = String.Empty

        If selectedUnit = DisplayUnits.Metric Then
            flow = "l/s"
            head = "m"
            power = "kW"
        ElseIf selectedUnit = DisplayUnits.Us Then
            flow = "USGPM"
            head = "ft"
            power = "kW"
        End If
        FlowLabel = String.Format(My.Resources.FlowLabel, flow)
        HeadLabel = String.Format(My.Resources.HeadLabel, head)
        PowerLabel = String.Format(My.Resources.PowerLabel, power)
    End Sub

The above code is used to set headers for a datagrid in WPF. The headertext is bound to FlowLabel, HeadLabel and PowerLabel. The code works but also feels bloated with too many line sof code for something seamingly trivial.

Finally...

 ShowQAxis1 = PerformanceTest.FlowScaleUnits <> DisplayUnits.Us
        ShowQAxis2 = PerformanceTest.FlowScaleUnits <> DisplayUnits.Metric
        ShowPAxis1 = PerformanceTest.PowerScaleUnits <> DisplayUnits.Us
        ShowPAxis2 = PerformanceTest.PowerScaleUnits <> DisplayUnits.Metric
        ShowHAxis1 = PerformanceTest.HeadScaleUnits <> DisplayUnits.Us
        ShowHAxis2 = PerformanceTest.HeadScaleUnits <> DisplayUnits.Metric

Two properties are used for each unit to determine if it is visible in a plot component. To set this I have to check the model object for each property. It is very teadious and looks like a lot of repeated code, or un-clever syntax.


Maybe the above code is fine and optimized, but I can't help but thinking that is a better way to solve it...

/H

[kebo]

Looking at what you have, you could probably make changes to reduce the line count, but I'm not sure you could do much more to make it "better" code (whatever that means). In all of your snippets, you still have to do your boolean checks and set the variables. I don't see where you are being redundant, so there is really nothing to be gained by changing it other than making line reductions at the cost of readability. Efficiency wise I doubt any changes would help. You may be able to use a bit-wise enumeration, but even with that, you would still need to do all of the boolean comparisons you are currently doing.
Kevin

[John_SC]

You can clean up the code a bit with a With Statement, and also since your if statement contains only a single statement, the End IF is not required

With PerformanceText
   If .TestResultUnits = 4 Then .TestResultUnits = 12
   If .FlowScaleUnits = 4 Then .FlowScaleUnits = 12
   If .HeadScaleUnits = 4 Then .HeadScaleUnits = 12
   If .PowerScaleUnits = 4 Then .PowerScaleUnits = 12
end with     
        End If

In the second block, having an ElseIF could cause problems unless you have an Else (what happens if it is not Metric or US units). If it truely is an either or case, then just make the ElseIF an Else without any condition. I don't see any easy way of further simplifying this.

In the third case you could

' the following assumes units are either US or Metric. No third option. 
' It also assumes units are all one type

Dim USUnits as boolen

USUnits = PerformanceTest.FlowScaleUnits <> DisplayUnits.Us

ShowQAxis1 = USUnits
ShowPAxis1 = USUnits
ShowHAxis1 = USUnits

ShowQAxis2 = not USUnits
ShowPAxis2 = not USUnits
ShowHAxis2 = not USUnits

You could also define a MetricUnits variable instead of using not USUnits

Alternatively, you might also be able to change the code to use arrays with length 2, with the zero index being metric and the first index being US units. Then you only need to check the unit type once and set and index (such as UnitType) to either 0 or 1. That might be cleaner.

Good luck.

[kebo]

Certainly the with statement in the first snippet would help.

In the third case you could...

In the last snippet, each axis is being shown using a different boolean comparison. You have simplified it to using a single comparison which changes the logic and the result.

[John_SC]

I agree. But I did have the caveat that all the units would be consistent. I also agree with your earlier comment that this is just tweeking around the edges. There might be a real tricky way of reducing the lines of code, but it would hurt readability.

[TnTinMN]

...The scenario is in the repository where an enum is converted to a number which is the type expected in the db, and sadly the db is not based on binary representation so I have to manually convert the value before saving the performanceTest object with Entity Framework

I do not wish to insult you, but this statement and the subsequent code example leads me to believe that you may not be using Enums properly. Enums are meant to allow you to use a symbolic representation for a value just like a variable but grouped in a logical way.

By this statement do you mean that the TestResultUnits , FlowScaleUnits , HeadScaleUnits, and PowerScaleUnits properties/fields on the performanceTest object are Enum types in your code and need their respective values mapped to new values that hold meaning in the database? If so, why not just redefine your program's Enums to reflect the database and use the symbolic Enum representation as it was intended versus using the Enum values in your code.

Before I go into any possibly unnecessary detail, please confirm or reject my above interpretation of the situation.

[MrNorth]

I do not wish to insult you, but this statement and the subsequent code example leads me to believe that you may not be using Enums properly. Enums are meant to allow you to use a symbolic representation for a value just like a variable but grouped in a logical way.

By this statement do you mean that the TestResultUnits , FlowScaleUnits , HeadScaleUnits, and PowerScaleUnits properties/fields on the performanceTest object are Enum types in your code and need their respective values mapped to new values that hold meaning in the database? If so, why not just redefine your program's Enums to reflect the database and use the symbolic Enum representation as it was intended versus using the Enum values in your code.

Before I go into any possibly unnecessary detail, please confirm or reject my above interpretation of the situation.

Hi!

Not feeling insulted at all I inherited this code from another developer who quitted, and now I am starting to make changes. This thing with the ...Units variable is like this:

In the database, these fields can have 3 different values 1 = Metrics, 2=US and 12= US & Metrics. Two checkboxes in the UI sets this.

For readability in the code, we felt that an enum would be a better way to represent this than an integer, because it improves readability for the developer.

We can then also use the Flags attribute for bitwise addition on the enum and use or and xor when checking and unchecking the checkboxes to combine the enums to either

US = 1 (one checkbox is checked)
Metrics = 2 (second checkbox is checked)
US | Metrics = 4 (both checkboxes are checked)

There is a small helper method that performs this...

If we could simply store the 4 in the database, it is my understanding (right or wrong?) that we don't need the code that checks if =4 and set it to 12, right?

I hope it answers your question? Maybe we should stick to numbers instead all the way, but it felt easier somehow to use enums with bit flags in the code, and then do the conversion just before saving the object to the db.

/H

[kaliman79912]

This is a little hack, performing the If statement is not much different than a simple math operation so it will not affect performance.

With PerformanceText
   .TestResultUnits = (.TestResultUnits + 6)\4
   .FlowScaleUnits = (.FlowScaleUnits + 6)\4
   .HeadScaleUnits = (.HeadScaleUnits + 6)\4
   .PowerScaleUnits = (.PowerScaleUnits + 6)\4
end with

It may not make the code smaller, easier to read, or faster, but it is cute. LOL

[TnTinMN]

That explanation makes it much more clear.

If we could simply store the 4 in the database, it is my understanding (right or wrong?) that we don't need the code that checks if =4 and set it to 12, right?

That is correct, but it appears that the database values of {1,2,12} are set and can not be changed and your choice of setting using the Enum as flags prevents setting the 'US | Metrics' to 12.

You could create a custom attribute that you would use to decorate the Enum fields. This custom attribute would be used to define the value stored in the database for the particular Enum value.

<Flags> _
Enum MyEnum As UInt32
   <StoredDBValueAttribute(1)> _
   US = 1
   <StoredDBValueAttribute(2)> _
   Metric = 2
   <StoredDBValueAttribute(12)> _
   Both = 4
End Enum

Friend Class StoredDBValueAttribute
   Inherits System.Attribute
   Public Sub New(dbValue As Int32)
      Me.DBValue = dbValue
   End Sub
   Public ReadOnly DBValue As Int32
End Class

This would necessitate some helper methods to handle the translation from Enum value to DB value. You may feel that the overhead of these methods is to large, but that is decision you would need to make.

Private Function GetDBValue(myEnumValue As MyEnum) As Int32
   Dim retValue As Int32 = 0
   Dim enumType As Type = GetType(MyEnum)
   If [Enum].IsDefined(enumType, myEnumValue) Then
      Dim name As String = [Enum].GetName(enumType, myEnumValue)
      Dim attribs As Object() = enumType.GetField(name).GetCustomAttributes(False)
      If attribs.Count > 0 Then
         Dim dbValueAttrib As StoredDBValueAttribute = attribs.OfType(Of StoredDBValueAttribute).FirstOrDefault
         If dbValueAttrib IsNot Nothing Then
            retValue = dbValueAttrib.DBValue
         End If
      End If
   Else
      Throw New ArgumentException("value is not a valid MyEnum")
   End If
   Return retValue
End Function

Private Function GetMyEnumFromDBValue(dbValue As Int32, ByRef retValue As MyEnum) As Boolean
   Dim ret As Boolean = False
   Dim enumType As Type = GetType(MyEnum)
   Dim enumValues As System.Array = [Enum].GetValues(enumType)
   For Each value As MyEnum In enumValues
      If GetDBValue(value) = dbValue Then
         retValue = value
         ret = True
         Exit For
      End If
   Next
   Return ret
End Function

[shoreteknow]

In your first bit of code, if you are only testing for one value and setting to something else then if is your best bet (as stated I believe), if you are testing for multiple values i.e. 4 = 12, 5=15, 6=18, et then use a case statement or use a helper function that takes an integer and returns an integer. This would make your code more flexible and handle possible future upgrades/options.

The helper function (or a property) would also apply to the third piece of code and use as a comparer function. It would take 2 parameters and return Boolean based on equality.

That is my 2 cents.

D

[MrNorth]

Thanks for all valuable comments, I have learned a lot more about VB.NET now and the with-operator which is very handy. I also had some ideas about keeping the code simple versus keeping it short, and will probably not make too many changes in the existing code.

Nice touch with the attributes, but it feels in my case like an overly complicated way to solve this since I have to introduce an enum flag which is not needed (the Both) and I still have to parse out the number in the repository, with a method instead of an IF. If I could somehow add enum support directly into entity framework and let it handle enum properties "automagically" it would feel more interesting. But still, thanks for the code I learned a lot by looking at it, and I will definetly keep it in my mind for future reference.

cheers
H

[dbasnett]

...This thing with the ...Units variable is like this:

In the database, these fields can have 3 different values 1 = Metrics, 2=US and 12= US & Metrics. Two checkboxes in the UI sets this...
/H

This is confusing. If you are using an enum for the values then US & Metrics should equal 3, not 12. If you can store 3 in the DB then all would be good. BTW - is the field in the DB an integer? This is what I imagine your code looks like:

    <Flags()> Public Enum Measure
        none = 0
        Metric = 1
        US = 2
        US_Metric = Metric Or US
    End Enum

    Private myMeasure As Measure = Measure.none

    Private Sub cbUS_CheckedChanged(sender As Object, e As EventArgs) Handles cbUS.CheckedChanged
        If cbUS.Checked Then
            setMeasure(Measure.US)
        Else
            clearMeasure(Measure.US)
        End If
    End Sub

    Private Sub cbMetric_CheckedChanged(sender As Object, e As EventArgs) Handles cbMetric.CheckedChanged
        If cbMetric.Checked Then
            setMeasure(Measure.Metric)
        Else
            clearMeasure(Measure.Metric)
        End If
    End Sub

    Private Sub setMeasure(whMeasure As Measure)
        myMeasure = myMeasure Or whMeasure
    End Sub

    Private Sub clearMeasure(whMeasure As Measure)
        myMeasure = myMeasure And (Measure.US_Metric Xor whMeasure)
    End Sub

    Private Function isMeasure(whMeasure As Measure) As Boolean
        Return (myMeasure And whMeasure) = whMeasure
    End Function

[TnTinMN]

...Nice touch with the attributes, but it feels in my case like an overly complicated way to solve this since I have to introduce an enum flag which is not needed (the Both)...

Just to be clear, the only reason that I decorated the Enum with the 'Flags' attribute is because you implied that you were using the Enum as a such. The 'Flags' attribute is no relation to applying other attributes.

However, as dbasnett noted, your value of '4' is not correct for the 'Both' or 'US_Metric' field in the implied usage and should be have a value of '3'.

I completely agree that the attribute route to add information is overkill for this case of mapping only three key-value pairs and it could probably be best handled with a Module (static class).