All Forums >> [Scripting] >> WSH & Client Side VBScript >> RE: looking for help in CreateObject("Wscript.Shell") Do you like VisualBasicScript.com? Link to us and help spread the word about our forum. Thanks!
Some remarks about desert_storm28's backup script:
For reference:
The proposed modifications won't make a running version out of the script; I use it only to anchor my ramblings at real world code.
Most of the remarks qualify as nitpicking/pedantic. I don't like to be regarded as a pain in something mentioned in DiGiTAL.SkReAM's signature and I'd prefer to get this "Sir" dropped, but I can live with that, if you have thought (perhaps even argued here) about a proposal before you dismiss it as 'not applicable in my situation' or 'inefficient' or 'to much bother'.
(1) Start with "Option Explicit"
(2) Separate main code and Subs/Functions strictly; add comment blocks before a sub/function definition to make the separation ostentatious
(3) If you have a "'personalization and modification starts here" section put it at the beginning of the script
(4) INDENT! your code
(5) Line 1 ff Creation of [System] Objects is costly and you need them all over the place; so initialize them once and for all as global variables early in the script.
I would prefer a naming convention/coding style like Dim goFSO : Set goFSO = CreateObject( "Scripting.FileSystemObject" ) because: (a) Using Option Explicit is better in the long run (b) the two statements in one line avoid that you have to check two possibly separated lines to ensure that you have a useful goFS (c) "g" hints at global (d) "o" and "FS" is shorter than "obj" and "FileSys" (which could mean "FileSystem" i.e. NTFS or FAT) There is no obligation to follow my rules, but try to follow consistent ones: If "objNetwork" than "objFileSys" If you create objFileSys here, than use it in line 229 ff too!
(6) Line 8 (and many others) (cf. 14) If you use the hungarian type prefix, use it correctly: "sAlert = 0" would qualify as fraud in my book. There is an article at http://www.joelonsoftware.com/articles/Wrong.html about this coding style. Read this, if you believe in "variable names should be prefixed with a type abbreviation" or if you think "that's for bean counters only".
(7) Line 12, 13 85 ff
12 '0=no sysstate backup 1=backup systate together w/ ".bkf" file 2=backup the syststate only 13 sSystemState = 0 ' red alert! Hungarian fraud!
sSystemState is used like this
85: If sSystemState = 1 Then 86: sBkfSysState 87: ntBackup sBkpParam1, sBkpParam2 88: ElseIf sSystemState = 2 Then sBkfSysState 89: ElseIf sSystemState = 0 Then ntBackup sBkpParam1, sBkpParam2
I'm buffled. If "0" means "".bkf" file", why not say so? When talking about conditions think and speak in positive mode! If "sBkfSysState" will backup Sys, than don't use "Bkf" in its name; if "ntBackup sBkpParam1, sBkpParam2" will backup Bkf than put a "Bkf" in.
(8) Line 26 ff sRemoteMedia<N> are very bad names. To use the odd number 1 as an indicator for the backup folder to be used on even days is a cunning trap. How about "sRMPathEven"?
(9) Line 39 ff Include the "." in the variables; then you can write: Set filetxt = fileSys.OpenTextFile(sLogPath & "\" & sLogName & sLogExt, ForAppending, True) instead of Set filetxt = fileSys.OpenTextFile(sLogPath & "\" & sLogName & "." & sLogExt, ForAppending, True) and avoid 4 chars of nasty noise.
(10) Line 46, 50, 51 Try to use exactly one timestamp of type Date Dim dtNow : dtNow = Now Use this variable to fill derived vars Dim dtToday : dtToday = DateValue( dtNow ) Dim sToday : sToday = CStr( dtToday ) Dim dtTime : dtTime = TimeValue( dtNow ) to avoid getting yesterday's date and today's time; Dim sLogname : sLogname = Replace( sToday, "/", "-" ) is risky, because you can't be always sure about the regional settings.
(11) Line 50 ff String concatenation in VBScript is clumsy; make it a bit more lucid by factoring out similarites and handle the differences systematically: sSubjFmt = "@0@: "& sLabel & " on " & objNetwork.ComputerName & " @1@ on " & dtNow ' not Now! sSubjFail = replaceList( sSubject0, Array( "FAIL" , "failed" ) ) sSubjSucc = replaceList( sSubject0, Array( "SUCCESS", "was successful" ) )
Function replaceList( ByVal sSrc, aRpls ) Dim nIdx ' the type prefix *is* redundant here! For nIdx = 0 To UBound( aRpls ) sSrc = Replace( sSrc, "@" & nIdx & "@", aRpls( nIdx ) Next replaceList = sSrc End Function
(12) Line 56 ff Don't let the building of your title string clutter up your main code; swap it out to a separate function that can be used in all your scripts. Then your main code could look like this
This sequence of code could be written in an early phase of the development process together with simple/faking stubbs for the subtasks. It would be stable as long as there are no design revisions - and even then changes would be easy. Try to do the equivalence of
to the start of the script (perhaps even before the config section), because you need the constants everywhere, not just in this sub. The "need to know/keep it in narrow scope" principle is a good idea, but it can conflict with "don't specify the same thing redundantly in many places".
Sub logMsg( sMsg ) Dim oTS : Set oTS = goFS.OpenTextFile( gsLogFSpec ... ) oTS.WriteLine sMsg ' yes, no ()! oTS.Close End Sub
If you accept a global variable for the file specification, consider:
Dim goTSLog : Set goTSLog = goFS.OpenTextFile( gsLogFSpec ... ) .... goTSLog.WriteLine sMsg .... goTSLog.Close WScript.Quit 0
Saving one Sub and many sub calls looks nice, but having a Sub allows enhancements like
Sub logMsg( sMsg ) goTSLog.WriteLine sMsg If gbShowOnScreen Then WScript.Echo sMsg End Sub
which are difficult if you use the inline approach.
(14) Line 85 ff Use "Select Case" instead of "If/ElseIf/Else" if you don't check completely different conditions; use names instead of numbers you'll have to lookup anyway:
Select Case sWhatToBackup Case "Sys" ntBackup sBkpParam1, sBkpParam2 Case "Bkf" sBkfSysState Case "Sys+Bkf" ntBackup sBkpParam1, sBkpParam2 sBkfSysState End Select
If you got lots of things to backup, consider
Dim aWhatToBackup = Array() ... ReDim Preserve aWhatToBackup( UBound( aWhatToBackup ) + 1 ) Array( UBound( aWhatToBackup ) ) = "Sys" ... ReDim Preserve aWhatToBackup( UBound( aWhatToBackup ) + 1 ) Array( UBound( aWhatToBackup ) ) = "Bkf" ... For Each sWhatToBackup in aWhatToBackup Select Case sWhatToBackup Case "Sys" .... End Select Next
"sWhatToBackup" and "aWhatToBackup" illustrate that even a 'type only' hungarian prefix can be valuable/handy: The name "WhatToBackup" hints at the common concept "Something to backup", the prefix differentiates between two aspects of this concept - the list of all the things to backup vs. the single thing currently to work on. There could be a "nWhatToBackup" if you want to count items. This works as long as the semantically not very meaningful/significient prefix suffices to keep the different aspects apart in the context of the concept.
Use "If/ElseIf/Else" instead of "Select Case" if you need check different conditions:
If A = B Or I <= J Then doThis ElseIf C > D And K <> L Then doThat Else Err.Raise 123, "If/ElseIf/Else", "logical break down" End If
To avoid falling thru to the Else part (nearly) every time it's advisable to do the evaluation of the boolean expression before the If branching - even if it means creating extra variables:
Dim bDoThis, bDoThat
bDoThis = A = B or bDoThis = (A = B) ' alas - another kind of () bDoThis = bDoThis Or (I <= J) ' this time used to make ' precedence stand out If gbDebug Then WScript.Echo "bDoThis", bDoThis ... If bDoThis Then doThis ElseIf bDoTha Then ...
(15) Line 90 ff Don't invest extra work in hiding problems. If you think you can rule out illegal values im sSystemState, don't pretend by .... Else Exit Sub End If that you did something meaningful to provide for this case. If you don't want to write .... Else WScript.Echo "***** Illegal sSystemState", sSystemState, "in Sub sBkf ******" WScript.Quit End If or .... Else Err.Raise vbObjectError + 4711, , "Sub sBkf", "Illegal sSystemState " & sSystemState End If write nothing.
(16) Line 116, 117, 149 ff Don't use () if you call a Sub, even if the VBScript Docs do it and some people don't care and you won't be punished each time you do it. The compiler and all caring programmers reading "backupFile (dest)" will understand that you want to pass "dest" per value to the Sub backupFile. If you don't want that, you are lying. You force every programmer (even yourself in two weeks) to think about the reason and to check Sub backupFile for its special magic. If a second parameter is added in a naive way "backupFile (dest, how)" you'll get an error or a script that fails silently (because of IExplorer's famous "show no errors" setting).
If you want to pass parameters by value, indicate it in the parameter list of the Function/Sub definition:
Function replaceList( ByVal sSrc, aRpls ) ' (cf. 11)
This way the fact will be noted once here and you don't need - or can forget - the () when calling the function.
Now I begin to realize why desert_storm28 decided to 'take some rest' after getting this script to work. So I will post this as it is and wait for some feedback before I decide whether to continue this tomorrow.
Sorry for taking me so long, I am trying now to modify the script together with your suggestions to make it look professional, but I'm afraid I could not do it, but I will try.
And I noticed something with my script, Im running this in one of our server, and Im getting the following error:
Script: d:\backup Line: 183 Char: 1 Error: The semaphore timeout period has expired.
and this always happens only in one server, and no problem with other servers.
I will make now the other version of the script, and I will post it here as soon I finished it.
Im posting the revision of my backup script, almost all suggestions of ehvbs has been implemented, but am still working on the others, specialy the factoring of semilarities, I can't get my desired result, I removed the blat utility to send an email, I replaced it with the built-in MS CDO, and also the log file has been converted to HTML. And also the following lines, am trying to pass the "sRoot" to another function, but I can't get it, I wanted the result to look something like this in the log file:
Removing the following old Recruitment Backup: "d:\backup\archive\7-20-2006_ComputerName_Recruitment.7z" "\\Server1\archive\7-20-2006_ComputerName_Recruitment.7z"
And below is the entire backup script:
note, 90% of the above codes was taken from this website, thank you for those who owned the code. and if you make your own revision of the above script, would appreciate if you could post it here... thanks!
And one thing, sometimes when the script is copying more than 1GB file to remote media, Im getting the following error message, but the file has been successfully copied, but I did not receive an email alert:
Script: d:\backup Line: Char: 1 Error: The semaphore timeout period has expired.
and it is pointing to the fololowing line:
and it happened to two servers already, but in other severs, had no errors like that when copying more than 1GB file.
< Message edited by desert_storm28 -- 7/22/2006 5:33:44 AM >
All Forums >> [Scripting] >> WSH & Client Side VBScript >> RE: looking for help in CreateObject("Wscript.Shell") 
Printable Version
RE: looking for help in CreateObject("Wscript.Shel... - 
New Messages
No New Messages
Hot Topic w/ New Messages
Hot Topic w/o New Messages
Locked w/ New Messages
Locked w/o New Messages
Post New Thread