Surely a more elegant way

[hog]

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]

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

[hog]

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]

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:

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]

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?

 

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

Edited April 17, 2003 by divil

[Nerseus]

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

 

-Nerseus

[hog]

Thnx......

[hog]

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?

[Derek Stone]

Use the following examples as a basis:

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

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

[Derek Stone]

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.

[hog]

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

[Derek Stone]

'### Assuming: Option Strict On, Imports System.Data.SqlClient ###

Dim oCommand As SqlCommand = New SqlCommand("SELECT * FROM 

[hog]

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

 

  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]

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:

      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

[Derek Stone]

I'd also change "Exception" to "SqlException".

[hog]

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]

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

 

' 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]

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

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]

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

 

Good one, thanks

[hog]

In the code below where I have the first catch will the return false staement result in the Finally block not getting called? Or does the Finally block get called regardless??

 

 

   Public Function Save() As Boolean

       Dim intX As Integer

       ' set internal error flag to false

       m_Error = False

       ' if validatedata returns false the data cannot be saved, user must recify

       If Not ValidateData() Then

           Return False

       End If

       ' if CalculateServiceDates returns false the data cannot be saved as service due dates are not created

       If Not CalculateServiceDates() Then

           Return False

       End If

       ' create a new command builder object based on this supplier objects data

       m_ocbContract = New System.Data.OleDb.OleDbCommandBuilder(m_odaContract)

       ' protect this section of code

       Try

           ' signal beginning of edit 

           m_drContract.BeginEdit()

           ' write the private members to the corresponding data column

           m_drContract.Item("pr_number") = m_PRNumber
           m_drContract.Item("budget") = m_BudgetCode
           m_drContract.Item("dept") = m_DepartmentCode
           m_drContract.Item("po_number") = m_PONumber
           m_drContract.Item("dated") = m_Dated
           m_drContract.Item("Commencement") = m_Commencement
           m_drContract.Item("expires") = m_Expiry
           m_drContract.Item("contact") = m_Contact
           m_drContract.Item("contact_tel") = m_ContactTelephone
           m_drContract.Item("price") = m_Cost
           m_drContract.Item("frequency") = m_Frequency
           m_drContract.Item("order_line") = m_OrderLine
           m_drContract.Item("description") = m_Detail
           m_drContract.Item("internal") = m_Internal

           ' cycle through array and store service due dates in table

           For intX = 0 To m_Frequency - 1

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

           Next

           ' signal end of edit

           m_drContract.EndEdit()

           ' get the required update command for this data 

           m_odaContract.UpdateCommand = m_ocbContract.GetUpdateCommand

           ' update the table in the database checking for concurrency errors while we are at it

           m_odaContract.Update(m_dsContract.Tables("tblContracts").GetChanges)

           ' signal to accept the changes

           m_dsContract.Tables("tblContracts").AcceptChanges()

           ' close the connection

           m_odaContract.UpdateCommand.Connection.Close()

       Catch objConcurrencyError As DBConcurrencyException

           ShowError("Location:   Class Contract" & ControlChars.CrLf & ControlChars.CrLf & "Procedure:  Save() As " & _
                     "Boolean" & ControlChars.CrLf & ControlChars.CrLf & "Error Text: " & "Another user has made " & _
                     "changes to this record since you opened it.  You must " & ControlChars.CrLf & _
                     "reload the record and try again.")

           ' set internal error flag to true

           m_Error = True

           Return False

       Catch objException As Exception

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

           ' set internal error flag to true

           m_Error = True

       Finally

           If m_odaContract.UpdateCommand.Connection.State.Open Then

               ' close the connection

               m_odaContract.UpdateCommand.Connection.Close()

           End If

       End Try

       ' return true to sigan data has been saved

       Return True

   End Function

[quwiltw]

The Finally block *always* gets executed.

[hog]

Cool......thx

[Derek Stone]

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?

Generally speaking if your code is erroneous you should fix it, not hand over its problems to a Try/Catch block. Some errors can arise due to reasons out of our hands, and accessing MS SQL Server is one of them. That doesn't mean that all errors in that block should be handled however. In your case only exceptions of type SqlException should be handled, since all other errors are most likely the result of poorly written code.

[hog]

Mmm point taken. However I'm using an Access database, not SQL Server so I presume then there is an exception of type Access or something?