Surely a more elegant way

hog · Apr 17, 2003

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

Nerseus · Apr 17, 2003

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, Ill need to know what youre trying to do (I dont know what m_Frequency is, for example).

-Ner

hog · Apr 17, 2003

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?

Nerseus · Apr 17, 2003

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 whats going on. For example:

Code:

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

hog · Apr 17, 2003

Ah, but it is in my code.....didnt 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?

Code:

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

Nerseus · Apr 17, 2003

Use the [ vb ] and [/ vb ] tags (minus the spaces). You can also use [ code ] for generic code and [ cs ] for C#.

-Nerseus

hog · Apr 17, 2003

Thnx......

hog · Apr 20, 2003

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 Im asking is, is there a more efficient way of doing it or does the phase, "if ir aint broke dont fix it" apply?

Derek Stone · Apr 20, 2003

Use the following examples as a basis:

Code:

### 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.

hog · Apr 20, 2003

OK thanks Derek, Ill 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 dont mean I should litter the block with multple try blocks, or is that how it is supposed to be done??

Thx

Derek Stone · Apr 20, 2003

You shouldnt 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.

hog · Apr 20, 2003

Mmm point taken. I trust however where code attempts to access the database then it should be protected?

Derek Stone · Apr 20, 2003

Code:

### 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

hog · Apr 21, 2003

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 Im learning....

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

thx

Code:

   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

Nerseus · Apr 21, 2003

Its a little large, but thats 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:

Code:

       If Not POTotal() Then

                Return

            End If

             close connection to database
            m_objConn.Close()

If you return when POTotal returns False, you wont close the connection.

-Nerseus

Derek Stone · Apr 21, 2003

Id also change "Exception" to "SqlException".

hog · Apr 22, 2003

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

hog · Apr 22, 2003

This is the modification Im making to all my code to fix the problem Nerseus pointed out:

Code:

 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

quwiltw · Apr 22, 2003

This may be getting into personal preference, but I like to put the close in the Finally block to ensure it always gets closed.

Code:

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

hog · Apr 22, 2003

Aha.....completely forgot about the Finally keyword..

Good one, thanks

Surely a more elegant way

Well-known member

Danner

Well-known member

Danner

Well-known member

Danner

Well-known member

Well-known member

Exalted One

Well-known member

Exalted One

Well-known member

Exalted One

Well-known member

Danner

Exalted One

Well-known member

Well-known member

Well-known member

Well-known member