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 maxWaitTimemaxWaitTime = 60 '1800 '30minspublic connLogFileNameconnLogFileName = connectionLog.txtpublic pathToRemoteServer pathToRemoteServer = c:\public errMessagepublic fsset 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 = nothingend sub'::::::::::::::::::::::::::::::sub createLogFile()  dim logName  logName = fs.GetAbsolutePathName(.) & \ & connLogFileName   If not fs.FileExists(logName) Then    fs.createTextFile logName, false  End Ifend sub ':::::::::::::::::::::::::::::::::::::::::function lineToWrite()  dim curUser  curUser = createObject(WScript.Network).userName  lineToWrite = curUser & :  & Now()     '& vbCrLfend 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 = Error on crt:  & err.description    err.clear    checkFolderPath = false    exit function  end if  aFile.WriteLine (This is a test file  & fn)   if err.Number <> 0 Then    errMessage = Error on wrt:  & err.description    err.clear    checkFolderPath = false    exit function  end if  aFile.Close   if err.Number <> 0 Then    errMessage = Error on cls:  & err.description    err.clear    checkFolderPath = false    exit function  end if      fs.DeleteFile fn   if err.Number <> 0 Then    errMessage = Error on del:  & err.description    err.clear    checkFolderPath = false    exit function  end if checkFolderPath = trueif not aFile is nothing then set aFile = nothingend function':::::::::::::::::::::::::::::::::::::::::function myDateFormat(aDate)  sc = second(aDate)  mn = minute(aDate)  hr = hour(aDate)  dy = day(aDate)  mt = month(aDate)      yr = year(aDate)  myDateFormat= yr & mt & dy & hr & mn & scend function
Checking if a remote server is online
vbscript
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 = 8This 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 = 8public maxWaitTimemaxWaitTime = 60 '1800 '30minspublic connLogFileNameconnLogFileName = connectionLog.txtNeither 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 errMessagepublic fsset 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 Ifend subSo, 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 Ifend 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)) thenWouldn't it read better as: if (folderPathExists(pathToRemoteServer)) thenSpeaking 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()     '& vbCrLfend functionI 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 FunctionAlso, you should remove dead, commented out code. It serves no purpose but to clutter things.One last thing. Your error messages kind of suck. It's one thing to abbreviate variable names, but it's an entire other matter to abbreviate error/log messages.errMessage = Error on crt:  & err.description errMessage = Error on cls:  & err.description errMessage = Error on del:  & err.descriptionDon't do that. Spell it out for whoever has to read that log.It's also noteworthy that there's a lot of copy paste in the CheckFolderPath function, as evidence by the snippet above. It's wholly worth while to extract that logic into a function that takes an Err object and a string in as arguments.