Surely a more elegant way

H

hog

Senior Contributor
Joined
Mar 17, 2003
Messages
984
Location
UK
To obtain the unrounded integer part of the division below

DateDiff(DateInterval.WeekOfYear, m_Commencement, m_Expiry, FirstDayOfWeek.Sunday) / m_Frequency)

Which results in 8.666666 rounding up to 9

If there a more elegant way than this?

dblTemp = CDbl(DateDiff(DateInterval.WeekOfYear, m_Commencement, m_Expiry, FirstDayOfWeek.Sunday) / m_Frequency)

intNumberOfWeeks = Math.Floor(dblTemp)

Thx:)
 
There might be a better way... what exactly are you trying to do? Meaning, do you *just* care about the rounding part? If so, using the Math library is the best option. Or, are you talking about your use of DateDiff, a VB-specific function? For that, I'll need to know what you're trying to do (I don't know what m_Frequency is, for example).

-Ner
 
OK here you go...

m_Commencement is the start date of a maintenance contract
m_Expiry is the end date of the contract
m_frequency is the service visit frequency

My object calculates a proposed set of service dates which the user can accept or amend.

So if the week difference is 52 and the frequency is 6 the result is 8.666666. This gets rounded up to 9 so my code calculates the dates at 9 week intervals between start and end, however the last date can end up being passed the end date of the contract.

So my bit of code early gives me 8 which then ensures my dates stay within the start/end boundary.

I look at the code though and think, yuk....must be a better way?
 
It looks pretty good except for lack of comments. For something this complex, it would be nice to break apart some of the code to more easily see what's going on. For example:
Visual Basic: 
Dim TotalWeeks As Integer
Dim NumWeeks As Double

' Find the number of weeks for this contract
TotalWeeks = DateDiff(DateInterval.WeekOfYear, m_Commencement, m_Expiry, FirstDayOfWeek.Sunday)

' Get the number of weeks for this contract
NumWeeks = CDbl(WeekDiff / m_Frequency)

' Round down the number of weeks so that the last
' week is guaranteed to be within the bounds of the contract
intNumberOfWeeks = Math.Floor(NumWeeks)

-Nerseus
 
Ah, but it is in my code.....didn't realise you wanted all that with it sorry? By the way how do you get code to appear in the white neat boxes on this web page?

Visual Basic: 
Dim intX As Integer

'holds the number of weeks between services

Dim intNumberOfWeeks As Integer

' stores date being calculated

Dim dtmCalculatedDate As Date

' temporary storage of division result

Dim dblTemp As Double

' set internal error flag to false

m_Error = False

' protect this block

Try

   ' resize the service date array to the selected frequency

   ReDim m_ServiceDates(m_Frequency, 2)

   ' calculate the number of weeks between commencement and expiry of contract and divide by frequency
   ' use floor method to obtain integer part without rounding up, which will result in the last service
   ' due date calculated being passed the expiry date

   dblTemp = CDbl(DateDiff(DateInterval.WeekOfYear, m_Commencement, m_Expiry, FirstDayOfWeek.Sunday) / m_Frequency)
   
   ' get the integer part

   intNumberOfWeeks = Math.Floor(dblTemp)

   ' seed calculated date

   dtmCalculatedDate = m_Commencement

   ' cycle through service date array

            For intX = 0 To m_Frequency - 1

                ' calculate the next service due date, add the required number of weeks to the date supplied

                dtmCalculatedDate = DateAdd(DateInterval.WeekOfYear, intNumberOfWeeks, dtmCalculatedDate)

                ' if the date falls on either a Saturday or a Sunday then subtract 2 days to make it fall on a work day

                If dtmCalculatedDate.DayOfWeek = DayOfWeek.Saturday Or dtmCalculatedDate.DayOfWeek = DayOfWeek.Sunday Then

                    dtmCalculatedDate = DateAdd(DateInterval.Weekday, -2, dtmCalculatedDate)

                End If

                ' store the date in the service date array and set serviced flag to false 

                m_ServiceDates(intX, 0) = dtmCalculatedDate
                m_ServiceDates(intX, 1) = "False"

            Next

    Catch objException As Exception

          ShowError("Location:   Class Contract" & ControlChars.CrLf & ControlChars.CrLf & _
                    "Procedure:  CalculateServiceDates() As Boolean" & ControlChars.CrLf & _
                    ControlChars.CrLf & "Error Text: " & objException.Message)

         ' set internal error flag to true

         m_Error = True

         Return False

    End Try
 
Last edited by a moderator:
Nerseus, maybe I used the wrong word, elegant? Perhaps I should have used efficient instead. As you see my code works fine, but the section where I get the result from the division then using a math method to obtain the integer portion without rounding seem messy to me. So what I'm asking is, is there a more efficient way of doing it or does the phase, "if ir ain't broke don't fix it" apply?
 
Use the following examples as a basis:
Visual Basic: 
'### Assuming: Option Strict On, Imports System ###

Dim dtCommencement As New DateTime(2003, 1, 1)
Dim dtExpiry As New DateTime(2003, 12, 31)

Dim tsTimespan As TimeSpan = dtExpiry.Subtract(dtCommencement)

Dim iWeeks As Integer = Convert.ToInt32(Math.Floor(tsTimespan.TotalDays / 7))
If you do nothing else at least remove the calls to CDbl, DateDiff and DateAdd. Also, the Try/Catch block you have going is far too large.
 
OK thanks Derek, I'll look into you suggestion re the function calls. Can you enlighten me about why my try blocks are too big? I thought the idea was to protect blocks of code that could bomb? Surely you don't mean I should litter the block with multple try blocks, or is that how it is supposed to be done??

Thx
 
You shouldn't need any, I repeat any, Try/Catch blocks with that code. People rely on error handling far too much, when in fact simple logic checks will alleviate most problems.
 
Mmm point taken. I trust however where code attempts to access the database then it should be protected?
 
Visual Basic: 
'### Assuming: Option Strict On, Imports System.Data.SqlClient ###
Dim oCommand As SqlCommand = New SqlCommand("SELECT * FROM [table]", New SqlConnection("Data Source=localhost;
"))

Try
    oCommand.Connection.Open()
Catch eException As SqlException
    'Connection failed to open
End Try

If oCommand.Connection.State = ConnectionState.Open Then
    'Perform actions
    oCommand.Connection.Close()
End If
 
Hi Derek,

this is the norm for how I use the try/catch block. I have been using .NET for four months now, migrating from VBA and Access. My last real use of true VB was VB5 so VB.NET is a full of new stuff I'm learning....

So is this block of code OK or messy???

thx

Visual Basic: 
   Public Sub New(ByVal strPONumber As String, ByVal lngSupplierID As Long)

        ' this constructor is used to create a contract object when the po and supplier name are known

        Dim intX As Integer

        ' set internal error flag to false

        m_Error = False

        ' create a connection using global connection string

        m_objConn = New System.Data.OleDb.OleDbConnection(gconnConnection)

        ' set-up the SQL which will return records for select PO number

        m_strSQL = "SELECT * FROM tblContracts WHERE tblContracts.po_number = '" & strPONumber & "' AND " & _
                   "tblContracts.Supplierid = " & lngSupplierID & "  ORDER BY " & _
                   "order_line "

        ' create a new data adapter for the required data

        m_odaContract = New System.Data.OleDb.OleDbDataAdapter(m_strSQL, m_objConn)

        ' protect this section of code

        Try

            ' open the connection to the required database

            m_objConn.Open()

            ' fill the data table

            m_odaContract.Fill(m_dsContract, "tblContracts")

            ' set the data row object to the first row of the table, there will be only 1 as the supplier table
            ' only allows unique entries

            m_drContract = m_dsContract.Tables("tblContracts").Rows(0)

            ' assign values to private members, these will becomae available through each properties methods
            ' assign nothing if database field contains no data

            m_ContractID = IIf(IsDBNull(m_drContract.Item("contractid")), Nothing, m_drContract.Item("contractid"))
            m_SupplierID = IIf(IsDBNull(m_drContract.Item("supplierid")), Nothing, m_drContract.Item("supplierid"))
            m_PRNumber = IIf(IsDBNull(m_drContract.Item("pr_number")), Nothing, m_drContract.Item("pr_number"))
            m_BudgetCode = IIf(IsDBNull(m_drContract.Item("budget")), Nothing, m_drContract.Item("budget"))
            m_DepartmentCode = IIf(IsDBNull(m_drContract.Item("dept")), Nothing, m_drContract.Item("dept"))
            m_PONumber = IIf(IsDBNull(m_drContract.Item("po_number")), Nothing, m_drContract.Item("po_number"))
            m_Dated = IIf(IsDBNull(m_drContract.Item("dated")), Nothing, m_drContract.Item("dated"))
            m_Commencement = IIf(IsDBNull(m_drContract.Item("commencement")), Nothing, m_drContract.Item("commencement"))
            m_Expiry = IIf(IsDBNull(m_drContract.Item("expires")), Nothing, m_drContract.Item("expires"))
            m_Contact = IIf(IsDBNull(m_drContract.Item("contact")), Nothing, m_drContract.Item("contact"))
            m_ContactTelephone = IIf(IsDBNull(m_drContract.Item("contact_tel")), Nothing, m_drContract.Item("contact_tel"))
            m_OrderLine = IIf(IsDBNull(m_drContract.Item("order_line")), Nothing, m_drContract.Item("order_line"))
            m_Detail = IIf(IsDBNull(m_drContract.Item("description")), Nothing, m_drContract.Item("description"))
            m_Cost = IIf(IsDBNull(m_drContract.Item("price")), Nothing, m_drContract.Item("price"))
            m_Frequency = IIf(IsDBNull(m_drContract.Item("frequency")), Nothing, m_drContract.Item("frequency"))
            m_Active = IIf(IsDBNull(m_drContract.Item("active")), Nothing, m_drContract.Item("active"))
            m_Internal = IIf(IsDBNull(m_drContract.Item("internal")), True, m_drContract.Item("internal"))
            m_EquipmentID = IIf(IsDBNull(m_drContract.Item("equipmentid")), Nothing, m_drContract.Item("equipmentid"))
            m_RecordCount = IIf(IsDBNull(m_dsContract.Tables("tblContracts").Rows.Count), Nothing, m_dsContract.Tables("tblContracts").Rows.Count)

            ' resize the service due date array to the frequency value

            ReDim m_ServiceDates(m_Frequency, 2)

            ' cycle through table and store service due dates in array

            For intX = 0 To m_Frequency - 1

                m_ServiceDates(intX, 0) = m_drContract.Item("s" & intX)
                m_ServiceDates(intX, 1) = m_drContract.Item("s" & intX & "done")

            Next

            ' call function to calculate the total po cost

            If Not POTotal() Then

                Return

            End If

            ' close connection to database

            m_objConn.Close()

        Catch objException As Exception

            ShowError("Location:   Class Contract" & ControlChars.CrLf & ControlChars.CrLf & _
                      "Procedure:  New(ByVal strPONumber As String, ByVal strSupplierID As Long)" & _
                      ControlChars.CrLf & ControlChars.CrLf & "Error Text: " & objException.Message)

            ' set internal error flag to true

            m_Error = True

        End Try

    End Sub
 
It's a little large, but that's not bad by itself. You can break out some of the code to a separate function if readability becomes an issue.

However, this part is bad:
Visual Basic: 
       If Not POTotal() Then

                Return

            End If

            ' close connection to database
            m_objConn.Close()

If you return when POTotal returns False, you won't close the connection.

-Nerseus
 
OK guys, thanks for this...

Derek you say change it to SQLException, would this mean then that any other form of error may be missed? I tend to use Exception in all my try blocks as a sort of catch all. Am I in the wrong here then?

Thx
 
This is the modification I'm making to all my code to fix the problem Nerseus pointed out:

Visual Basic: 
' call function to calculate the total po cost

If Not POTotal() Then

	If m_objConn.State.Open Then

      	m_objConn.Close()

      End If

     Return

End If
 
This may be getting into personal preference, but I like to put the close in the Finally block to ensure it always gets closed.
Visual Basic: 
Try
    'do your stuff
Catch
    ' catch your exception
Finally
      If Not m_objConn Is Nothing AndAlso m_objConn.State.Open Then
          m_objConn.Close()
      End If
End Try
 
Back
Top