Checking if a remote server is online

I have this vbs file to check if a remote server is accessible. Can I simplify or refactor to make it more robust/efficient?

```
const forReading = 1, forWriting = 2, forAppending = 8
public maxWaitTime
maxWaitTime = 60 '1800 '30mins
public connLogFileName
connLogFileName = "connectionLog.txt"
public pathToRemoteServer
pathToRemoteServer = "c:\"
public errMessage
public fs
set fs = createObject("scripting.fileSystemObject")

pathToRemoteServer = "c:\"

'::::::::::::::::::::::::::::::
call mainSub()

'::::::::::::::::::::::::::::::
sub mainSub()
call createLogFile()

errMessage = ""
dim curDir
curDir = fs.GetAbsolutePathName(".")
dim logName
logName = curDir & "\" & connLogFileName
Dim aFile
set aFile = fs.openTextFile(logName, forAppending, True)

if (checkFolderPath(pathToRemoteServer)) then
aFile.WriteLine lineToWrite & " ok"
else
aFile.WriteLine lineToWrite & " " & errMessage

dim downTime
downTime = 0
do Until (checkFolderPath(pathToRemoteServer)) or downTime => maxWaitTime
WScript.Sleep 500 'half second
downTime = downTime + 0.5
loop

aFile.WriteLine lineToWrite & " back after seconds: " & downTime

end if

aFile.close
if not aFile is nothing then set aFile = nothing
end sub

'::::::::::::::::::::::::::::::
sub createLogFile()
dim logName
logName = fs.GetAbsolutePathName(".") & "\" & connLogFileName
If not fs.FileExists(logName) Then
fs.createTextFile logName, false
End If
end sub

':::::::::::::::::::::::::::::::::::::::::
function lineToWrite()
dim curUser
curUser = createObject("WScript.Network").userName
lineToWrite = curUser & ": " & Now() '& vbCrLf
end function

':::::::::::::::::::::::::::::::::::::::::
function checkFolderPath(folder_path)
checkFolderPath = false
On Error Resume Next

dim fn
fn = folder_path & "testfile." & myDateFormat(Now())

dim aFile
set aFile = fs.createTextFile(fn, True)
if err.Number <> 0 Then
errMessage =

There's a lot to cover and talk about here. I'm going to take a line by line approach, so this review might seem nitpicky (and it probably is), but there are some seemingly innocuous things here that can cause bigger problems given a chance.

const forReading = 1, forWriting = 2, forAppending = 8

This is good. I like well named constants. These are implicitly Public though. That means they're available to all scripts. Which is also okay. I see no reason these can't be, but it's always better to be explicit and declare the scope.

Public Const forReading = 1, forWriting = 2, forAppending = 8

public maxWaitTime
maxWaitTime = 60 '1800 '30mins
public connLogFileName
connLogFileName = "connectionLog.txt"

Neither of these should be Public. Nor should they be variables. They're constants and should be declared as such. Let the compiler warn you if you ever try to assign a new value to these.

public pathToRemoteServer 
pathToRemoteServer = "c:\"
public errMessage
public fs
set fs = createObject("scripting.fileSystemObject")

pathToRemoteServer = "c:\"

'::::::::::::::::::::::::::::::

Same advice here. These variables should be private to the script. Also, you're reassigning the same value to the same variable just a few lines apart?? Make it a constant and forget it.

I try not to pick on names, but fs would be better named fso. The latter seems to be an idiom in the community. Generally, I don't recommend overly abbreviated names like this, but idioms are ok. VBA/VBScript developers will recognize both of these as a FileSystemObject.

'::::::::::::::::::::::::::::::
call mainSub()

'::::::::::::::::::::::::::::::
sub mainSub()

Two things jump out at me here.

• mainSub isn't a good name. It doesn't say anything at all about what the subroutine does.

• It doesn't seem to make much sense to have a subroutine here at all. You could remove the Sub...End Sub statement and just execute it in your main script without changing the semantics or execution at all. (I don't recommend actually doing that, but I'm not quite there yet.)

sub mainSub()
  call createLogFile()

  errMessage = ""
  dim curDir
  curDir = fs.GetAbsolutePathName(".")
  dim logName
  logName = curDir & "\" & connLogFileName
' ....
End Sub

'::::::::::::::::::::::::::::::
sub createLogFile()
  dim logName
  logName = fs.GetAbsolutePathName(".") & "\" & connLogFileName 
  If not fs.FileExists(logName) Then
    fs.createTextFile logName, false
  End If
end sub

So, you're going to your file system object to get the current directory twice in rapid succession and sharing a variable between these two methods. That's smelly to me. createLogFile should take in a full file path as an argument. Then, connLogFileName doesn't need to be scoped outside of mainSub. Actually, I don't see a need for that constant at all in this case. You can keep it if you want, but I've removed it below.

sub createLogFile(logName)
  If not fs.FileExists(logName) Then
    fs.createTextFile logName, false
  End If
end sub 

sub mainSub()

  errMessage = ""

  dim logName
  logName = fs.GetAbsolutePathName(".") & "\" & "connectionLog.txt"

  call createLogFile(logName)

  Dim aFile
  set aFile = fs.openTextFile(logName, forAppending, True)

I also think it would be worthwhile to pass pathToRemoteServer into mainSub as an argument. It makes the sub more re-usable, but I would definitely think up a decent name for it.

I would rename checkFolderPath. Consider this snippet.

if (checkFolderPath(pathToRemoteServer)) then

Wouldn't it read better as:

if (folderPathExists(pathToRemoteServer)) then

Speaking of method names, I really recommend PascalCased method names. It distinguishes them from camelCased variable names.

I'm surprised this compiles.

downTime => maxWaitTime

In VBA this results in a compiler error and it's expected to be downTime >= maxWaitTime. Perhaps VBScript is more forgiving, but it also looks like the lambda operator from another popular Microsoft language. So two things happen here. I can't paste this into the VBA Editor and run it and it could cause confusion for someone expecting this to be the lambda operator.

':::::::::::::::::::::::::::::::::::::::::
function lineToWrite()
  dim curUser
  curUser = createObject("WScript.Network").userName
  lineToWrite = curUser & ": " & Now()     '& vbCrLf
end function

I like this function and the abstraction it provides, but you are calling it over and over and over to get the same string. Declare a variable and call this function once. Also, as an option, It would be completely acceptable to remove the curUser variable and one line this function.

Function LineToWrite()

    LineToWrite = CreateObject("WScript.Network").userName & ": " & Now()  

End Function

Also, you should remove dead, commented out code. It serves no purpose but to clutter things.

One last thing. Your error messages k

StackExchange Code Review Q#77173, answer score: 2