snippetMinor
How to make this ping test with timer more efficient?
Viewed 0 times
thiswithmakemoreefficientpingtimertesthow
Problem
I wrote a small ducktaped VBA script which pings servers every 15 mins or so. If a server's status is anything other than "Alive", the server and timestamp is written to another worksheet called "Log".
Is there another way (or better way) to do the countdown timer?
Sub Countup()
Dim CountDown As Date
CountDown = Now + TimeValue("00:00:01")
Application.OnTime CountDown, "Auto_Open"
End Sub
Sub Auto_Open()
Dim count As Range
Set count = Worksheets("Servers").Range("A1:A1")
count.Value = count.Value - TimeSerial(0, 0, 1)
If count "" Then
'If Cells(i, 2) = "Request Timed Out" Or Cells(i, 2) = "" Or Cells(i, 2) = "Dead" Then
Cells(i, 2) = sPing(Cells(i, 1))
Cells(i, 3) = Now()
'log it to Log
If Cells(i, 2).Value <> "Alive" Then
Call copytest(i)
End If
'End If
'End If
i = i + 1
Loop
Set applicationobject = Nothing
End Sub
Function findlast_Row() As Long
Dim ws As Worksheet
Set ws = ThisWorkbook.Sheets("Log")
With ws
findlast_Row = .Range("A" & .Rows.count).End(xlUp).Row
End With
End Function
Sub copytest(ByVal intRow As Integer)
'screens for last row in log sheet
iLastRow = findlast_Row() + 1
Worksheets("Log").Range("A" & CStr(iLastRow) & ":E" & CStr(iLastRow)).Value = Worksheets("Servers").Range("A" & CStr(intRow) & ":E" & CStr(intRow)).Value
End SubIs there another way (or better way) to do the countdown timer?
Solution
Using
That said, your naming and access modifiers lack consistency (I guess that's what you meant with "duct-taped"?).
Observations
Nitpicks
I know this is an incomplete review, but I think you have enough meat here anyway.
Application.OnTime is a very neat way of implementing your timer, but I have a hard time figuring out how "pings servers every 15 mins or so" translates to Now + TimeValue("00:00:01"). Note that since VBA is single-threaded, OnTime doesn't mean your code will run at that specific time, rather that when Excel isn't busy doing something else, it will queue your method for synchronous execution, like any other event handler code.That said, your naming and access modifiers lack consistency (I guess that's what you meant with "duct-taped"?).
Observations
- You're using
ListObject.ListRows.Count, but you're puzzling your way into adding a new row - why aren't you usingListObject.ListRows.Add()which returns the added row?
- In
sPingfunction (awful name), I would refactor thisElseIfconstruct into somePrivate Function GetPingResult(StatusCode As Long) As String, and assign the function's return value once; also you made the default value "Alive" - if there's an error code you haven't accounted for, your function returns "Alive" which is possibly wrong.
Nitpicks
- This is VBA - stick to PascalCasing for all identifiers. It will make your code read consistently with the language itself.
- The default access modifier for
SubandFunctionisPublic- thus, either specify it or leave it out, but don't do both. If the unspecified ones are supposed to bePrivate, be explicit about it.
- The language's convention for underscores in procedure names, is only for methods that implement interface methods (e.g. event handlers). Avoid it.
- Worse than Hungarian notation (
strTableName As String), is inconsistent Hungarian notation, especially in the same method signature (why isn't itiCol As Integerthen?). Avoid it (Hungarian notation, that is!).
I know this is an incomplete review, but I think you have enough meat here anyway.
Context
StackExchange Code Review Q#25189, answer score: 6
Revisions (0)
No revisions yet.