Coding Practices, Best methods?

Coding Practices, Best methods?

Post by Matthe » Thu, 21 Sep 2006 01:09:26


am somewhat a novice when it comes to vbscripts. I know enough to
take most scripts and modify them to fit my needs. However I tend to
use brute force tactics when programming them. (quick and dirty
methods)

Below is a script that I hacked together. It in not very elegant or
efficient and I am looking for ways to improve on it.

I would appreciate if someone would take the time and review the code
and see where I can make improvements. Please give an example if you
can.

TIA

-Matt-

ON ERROR RESUME NEXT
Dim AdCn, AdRec, AdRec1
Set AdCn = CreateObject("ADODB.Connection")
Set AdRec = CreateObject("ADODB.Recordset")
Set AdRec1 = CreateObject("ADODB.Recordset")
' NOTE: Change the connection string according to your environment.
AdCn.Open = "Provider=SQLOLEDB.1; Data Source=SDLC; Integrated
Security=SSPI; Persist SecurityInfo=False; Initial Catalog=DBADMIN_Dev;
user id=diskuser; password=disk"
SQL1 = "SELECT SystemName FROM System_Monitor_List WHERE MonitorEnabled
= 1"
'wscript.echo SQL1 ' Debugging
AdRec1.Open SQL1, AdCn,1,1
DO UNTIL AdRec1.EOF
Computer = AdRec1("SystemName")
' wscript.echo Computer ' Debugging
SET objWMIService = GetObject("winmgmts://" & Computer)
IF err.number <> 0 THEN
ErrorSQL="EXEC SysMon_Log_Errors " & "'" & Computer & "','" &
Err.description & "'"
' wscript.echo "Start Error :" & ErrorSQL ' Debugging
AdRec.Open ErrorSQL, AdCn,1,1
ELSE
'HotFix Information
SET Win32_QuickFixEngineering_colItems =
objWMIService.ExecQuery("SELECT * FROM Win32_QuickFixEngineering",,48)
FOR EACH objItem IN Win32_QuickFixEngineering_colItems
IF err.number <> 0 THEN
ErrorSQL="EXEC SysMon_Log_Errors " & "'" & Computer & "','" &
Err.description & "'"
' wscript.echo "Loop Error: " & ErrorSQL ' Debugging
AdRec.Open SQL, AdCn,1,1
ELSEIF objItem.HotFixID <> "File 1" THEN
SQL="EXEC SysMon_Win32_QuickFixEngineering " & "'" & Computer &
"','" & objItem.HotFixID & "','" & objItem.Description & "','" &
objItem.InstalledBy & "','" & objItem.InstalledOn & "'"
' wscript.echo sql ' Debugging
AdRec.Open SQL, AdCn,1,1
END IF
NEXT
'Memory Information
SET Win32_PhysicalMemory_colItems = objWMIService.ExecQuery("SELECT *
FROM Win32_PhysicalMemory",,48)
FOR EACH objItem IN Win32_PhysicalMemory_colItems
IF err.number <> 0 THEN
ErrorSQL="EXEC SysMon_Log_Errors " & "'" & Computer & "','" &
Err.description & "'"
' wscript.echo "Loop Error: " & ErrorSQL ' Debugging
AdRec.Open SQL, AdCn,1,1
ELSE
SQL="EXEC SysMon_Win32_PhysicalMemory " & "'" & Computer & "','" &
objItem.Capacity & "','" & objItem.DeviceLocator & "','" &
objItem.FormFactor & "','" & objItem.MemoryType & "','" & objItem.Speed
& "'"
' wscript.echo sql ' Debugging
AdRec.Open SQL, AdCn,1,1
END IF
NEXT
'Processor Information
Set Win32_Processor_colItems = objWMIService.ExecQuery("SELECT * FROM
Win32_Processor",,48)
FOR EACH objItem IN Win32_Processor_colItems
IF err.number <> 0 THEN
ErrorSQL="EXEC SysMon_Log_Errors " & "'" & Computer & "','" &
Err.description & "'"
' wscript.echo "Loop Error: " & ErrorSQL ' Debugging
AdRec.Open SQL, AdCn,1,1
ELSE
SQL="EXEC SysMon_Win32_Processor " & "'" & Computer & "','" &
objItem.L2CacheSize & "','" & objItem.MaxClockSpeed & "','" &
objItem.Name & "'"
' wscript.echo sql ' Debugging
AdRec.Open SQL, AdCn,1,1
END IF
NEXT
END I
 
 
 

Coding Practices, Best methods?

Post by McKiraha » Thu, 21 Sep 2006 02:43:38

Matthew" < XXXX@XXXXX.COM > wrote in message
news: XXXX@XXXXX.COM ...


Will this help?

I have no idea it it will run; I just cleaned up the code.

Watch for word-wrap; expecially with "Const cDSN =".

Option Explicit
'*
'* Declare Constants
'*
Const cVBS = "WMI_EXEC.vbs"
Const cDSN = "Provider=SQLOLEDB.1;Data Source=SDLC;Integrated
Security=SSPI;Persist SecurityInfo=False;Initial Catalog=DBADMIN_Dev;user
id=diskuser;password=disk"
'*
'* Declare Globals
'*
Dim intSQL
intSQL = 0
Dim strSQL
strSQL = "SELECT SystemName FROM System_Monitor_List WHERE
MonitorEnabled = 1"
'*
'* Declare Objects
'*
Dim objADO
Set objADO = CreateObject("ADODB.Connection")
objADO.Open = cDSN
Dim objRS1
Set objRS1 = CreateObject("ADODB.Recordset")
objRS1.Open strSQL,objADO,1,1
Dim objRS2
Set objRS2 = CreateObject("ADODB.Recordset")
'*
'* Process each Computer
'*
Do Until objRS1.EOF
Process(objRS1("SystemName").Value)
objRS1.MoveNext
Loop
'*
'* Destroy Objects
'*
objRS2,Close
Set objRS2 = Nothing
objRS1.Close
Set objRS1 = Nothing
objADO.Close
Set objADO = Nothing
'*
'* Finish Message
'*
MsgBox intSQL & " EXECs",vbInformation,cVBS

Sub Process(Computer)
'****
'* Process each Computer
'****
If Computer = "" Then Exit Sub
On Error Resume Next
'*
'* Declare Variables
'*
Dim objItem
Dim objWMI
'*
'* WMIService
'*
Set objWMIService = GetObject("winmgmts://" & Computer)
If Err.Number <> 0 Then
Call Results(Computer,Err.Number,Err.Description,"")
Else
'*
'* HotFix Information
'*
strSQL = "SELECT * FROM Win32_QuickFixEngineering"
Set objWMI = objWMIService.ExecQuery(strSQL,,48)
For Each objItem In objWMI
strSQL = "EXEC SysMon_Win32_QuickFixEngineering '" _
& Computer & "','" _
& objItem.HotFixID & "','" _
& objItem.Description & "','" _
& objItem.InstalledBy & "','" _
& objItem.InstalledOn & "'"
Call
Results(Computer,Err.Number,Err.Description,objItem.HotFixID)
Next
'*
'* Memory Information
'*
strSQL = "SELECT * FROM Win32_PhysicalMemory"
Set objWMI = objWMIService.ExecQuery(strSQL,,48)
For Each objItem In objWMI
strSQL = "EXEC SysMon_Win32_PhysicalMemory '" _
& Computer & "','" _
& objItem.Capacity & "','" _
& objItem.DeviceLocator & "','" _
& objItem.FormFactor & "','" _
& objItem.MemoryType & "','" _
& objItem.Speed & "'"
Call Results(Computer,Err.Number,Err.Description,"")
Next
'*
'* Processor Information
'*
strSQL = "SELECT * FROM Win32_Processor"
Set objWMI = objWMIService.ExecQuery(strSQL,,48)
For Each objItem In objWMI
strSQL = "EXEC SysMon_Win32_Processor '" _
& Computer & "','" _
& objItem.L2CacheSize & "','" _
& objItem.MaxClockSpeed & "','" _
& objItem.Name & "'"
Call R
 
 
 

Coding Practices, Best methods?

Post by McKiraha » Thu, 21 Sep 2006 03:03:03

McKirahan" < XXXX@XXXXX.COM > wrote in message
news: XXXX@XXXXX.COM ...

I forgot to declare objWMIService in my last post.

Here's a better version which I tested without the ADO code.

Option Explicit
'*
'* Declare Constants
'*
Const cVBS = "WMI_EXEC.vbs"
Const cDSN = "Provider=SQLOLEDB.1;Data Source=SDLC;Integrated
Security=SSPI;Persist SecurityInfo=False;Initial Catalog=DBADMIN_Dev;user
id=diskuser;password=disk"
'*
'* Declare Globals
'*
Dim intSQL
intSQL = 0
Dim strSQL
strSQL = "SELECT SystemName _
FROM System_Monitor_List _
WHERE MonitorEnabled = 1"
'*
'* Declare Objects
'*
Dim objADO
Set objADO = CreateObject("ADODB.Connection")
objADO.Open = cDSN
Dim objRS1
Set objRS1 = CreateObject("ADODB.Recordset")
objRS1.Open strSQL,objADO,1,1
Dim objRS2
Set objRS2 = CreateObject("ADODB.Recordset")
'*
'* Process each Computer
'*
Do While Not objRS1.EOF
Process(objRS1("SystemName").Value)
objRS1.MoveNext
Loop
'*
'* Destroy Objects
'*
objRS2,Close
Set objRS2 = Nothing
objRS1.Close
Set objRS1 = Nothing
objADO.Close
Set objADO = Nothing
'*
'* Finish Message
'*
MsgBox intSQL & " EXECs",vbInformation,cVBS

Sub Process(Computer)
'****
'* Process each Computer
'****
If Computer = "" Then Exit Sub
On Error Resume Next
'*
'* Declare Variables
'*
Dim objITM
Dim objQRY
Dim objWMI
'*
'* WMIService
'*
Set objWMI = GetObject("winmgmts://" & Computer)
If Err.Number <> 0 Then
Call Results(Computer,Err.Number,Err.Description,"")
Else
'*
'* HotFix Information
'*
strSQL = "SELECT * FROM Win32_QuickFixEngineering"
Set objQRY = objWMI.ExecQuery(strSQL,,48)
For Each objITM In objQRY
strSQL = "EXEC SysMon_Win32_QuickFixEngineering '" _
& Computer & "','" _
& objITM.HotFixID & "','" _
& objITM.Description & "','" _
& objITM.InstalledBy & "','" _
& objITM.InstalledOn & "'"
Call
Results(Computer,Err.Number,Err.Description,objITM.HotFixID)
Next
'*
'* Memory Information
'*
strSQL = "SELECT * FROM Win32_PhysicalMemory"
Set objQRY = objWMI.ExecQuery(strSQL,,48)
For Each objITM In objQRY
strSQL = "EXEC SysMon_Win32_PhysicalMemory '" _
& Computer & "','" _
& objITM.Capacity & "','" _
& objITM.DeviceLocator & "','" _
& objITM.FormFactor & "','" _
& objITM.MemoryType & "','" _
& objITM.Speed & "'"
Call Results(Computer,Err.Number,Err.Description,"")
Next
'*
'* Processor Information
'*
strSQL = "SELECT * FROM Win32_Processor"
Set objQRY = objWMI.ExecQuery(strSQL,,48)
For Each objITM In objQRY
strSQL = "EXEC SysMon_Win32_Processor '" _
& Computer & "','" _
& objITM.L2CacheSize & "','" _
& objITM.MaxClockSpeed & "','" _
& objITM.Name & "'"
Call Resu
 
 
 

Coding Practices, Best methods?

Post by Richard Mu » Thu, 21 Sep 2006 03:08:06

i,

I also had to clean up the code before I could even start. If I can't read,
I can't understand it. I would never use "On Error Resume Next" as it makes
troubleshooting nearly impossible. I also use "Option Explicit", again so I
don't waste time troubleshooting. My version of the cleaned up code follows,
but I have 2 possible errors.

1. First, in a few places you assign a value to ErrorSQL, but use SQL
instead. I fixed that.
2. I thought you always had to close a recordset before you opened it again.
I assume in my "fixed" version that you will get errors similar to "Open not
allowed when recordset open". If so, make sure you always close your
recordsets when they are no longer needed.

I tried to trap errors only where they are expected. In my experience, the
WMI query statements might raise an error, but if they do not, the For Each
loop does not raise an error. I don't know what your stored procedures do.

I added constants for your CursorType and LockType, so others don't have to
look it up like I did.

My version (watch line wrapping):
=============
Option Explicit

Dim AdCn, AdRec, AdRec1, objItem
Dim SQL1, SQL, ErrorSQL, Computer, objWMIService
Dim Win32_QuickFixEngineering_colItems
Dim Win32_PhysicalMemory_colItems
Dim Win32_Processor_colItems

Const adOpenKeySet = 1
Const adlockReadOnly = 1

Set AdCn = CreateObject("ADODB.Connection")
Set AdRec = CreateObject("ADODB.Recordset")
Set AdRec1 = CreateObject("ADODB.Recordset")

' NOTE: Change the connection string according to your environment.
AdCn.Open = "Provider=SQLOLEDB.1;" _
& "Data Source=SDLC;" _
& "Integrated Security=SSPI;" _
& "Persist SecurityInfo=False;" _
& "Initial Catalog=DBADMIN_Dev;" _
& "user id=diskuser;" _
& "password=disk"

SQL1 = "SELECT SystemName " _
& "FROM System_Monitor_List " _
& "WHERE MonitorEnabled = 1"

AdRec1.Open SQL1, AdCn, adOpenKeySet, adLockReadOnly

DO UNTIL AdRec1.EOF
Computer = AdRec1.Fields("SystemName").Value
On Error Resume Next
SET objWMIService = GetObject("winmgmts://" & Computer)
IF err.number <> 0 THEN
On Error GoTo 0
ErrorSQL="EXEC SysMon_Log_Errors " _
& "'" & Computer & "','" & Err.description & "'"
AdRec.Open ErrorSQL, AdCn, adOpenKeySet, adLockReadOnly
ELSE
On Error GoTo 0
'HotFix Information
On Error Resume Next
SET Win32_QuickFixEngineering_colItems = _
objWMIService.ExecQuery("SELECT * FROM
Win32_QuickFixEngineering",,48)
IF err.number <> 0 THEN
On Error GoTo 0
ErrorSQL="EXEC SysMon_Log_Errors " _
& "'" & Computer & "','" & Err.description & "'"
AdRec.Open ErrorSQL, AdCn, adOpenKeySet, adLockReadOnly
Else
On Error GoTo 0
FOR EACH objItem IN Win32_QuickFixEngineering_colItems
IF objItem.HotFixID <> "File 1" THEN
SQL="EXEC SysMon_Win32_QuickFixEngineering " _
& "'" & Computer & "','" & objItem.HotFixID _
& "','" & objItem.Description & "','" _
& objItem.InstalledBy & "','" _
& objItem.InstalledOn & "'"
AdRec.Open SQL, AdCn, adOpenKeySet, adLockReadOnly
END IF
NEXT
End If
'Memory Information
 
 
 

Coding Practices, Best methods?

Post by Matthe » Thu, 21 Sep 2006 03:44:50

hank you so much. This was more then I was hoping for. It will take me
a while to digest the changes, but I will work my way though it. The
vbs is part of a larger script that goes out to a bunch of systems on a
network and logs in information.

Once again thank you.

-Matt-

McKirahan wrote:
> http://www.devguru.com/technologies/ado/8686.asp> >> > ADO Examples and Best Practices> > http://www.serverwatch.com/tutorials/article.php/1475391> >> >> >> > I standardized most variable names; for example> > intSQL, strSQL, objADO, etc.> >> > I standardized the capitalization of VBScript commands:> > Const, Dim, Do, Loop, For, Next, If Else, ElseIf, Then, End If, etc.> >> > I used Subs to minimize redundancy.> > > > I used (minimal) comments to make the code more readable.

 
 
 

Coding Practices, Best methods?

Post by Richard Mu » Thu, 21 Sep 2006 03:53:45

Hi,

An ADO recordset object is used to run a query and return a recordset. I
don't recall seeing it used to run a stored procedure. In your program, they
do not seem to return a recordset, or at least you don't use the recordset.

I believe it makes more sense to run the stored procedures with the Execute
method of an ADO command object. Then there is no need to close a recordset.

--
Richard
Microsoft MVP Scripting and ADSI
Hilltop Lab - http://www.yqcomputer.com/
 
 
 

Coding Practices, Best methods?

Post by Matthe » Thu, 21 Sep 2006 22:36:32

i McKirahan,
Thank you for the help so far. I am using your script as a guide and it
worked great so far, however I do have a question. I noticed while
debugging that it seems to process the query twice. I don't know what
the impact of this might be; I have confirmed that the results are the
same. Perhaps it's the different flow that is throwing me off.

Hi Richard,
I am using McKirahan format as a template, mainly because I feel it is
more modular, and I am planning to add other WMI calls, in addition the
idea of having a all the error handling in a separate process seem more
beneficial. You are correct I am using Stored Procedures to process the
incoming information. SQL should handle this process faster then VB (or
that's what some people say) and more secure.

I would still like to know if there is a better way for making the
database calls, and if you have any alternative suggestions.

Thanks.

-Matt-

Here is the code I am using, its more or less the same as the original
with the exception I added another item to the Processor routine and
add I am now sending the error code when an error occurs

[Code]
Option Explicit
'*
'* Declare Constants
'*
Const cVBS = "New.vbs"
Const cDSN = "Provider=SQLOLEDB.1;Data Source=SDLC;Integrated
Security=SSPI;Persist SecurityInfo=False;Initial
Catalog=DBADMIN_Dev;user id=diskuser;password=disk"
'*
'* Declare Globals
'*
Dim intSQL
intSQL = 0
Dim strSQL
strSQL = "SELECT SystemName FROM System_Monitor_List WHERE
MonitorEnabled = 1"
'*
'* Declare Objects
'*
Dim objADO
Set objADO = CreateObject("ADODB.Connection")
objADO.Open = cDSN
Dim objRS1
Set objRS1 = CreateObject("ADODB.Recordset")
objRS1.Open strSQL,objADO,1,1
Dim objRS2
Set objRS2 = CreateObject("ADODB.Recordset")
'*
'* Process each Computer
'*
Do While Not objRS1.EOF
Process(objRS1("SystemName").Value)
objRS1.MoveNext
Loop
'*
'* Destroy Objects
'*
objRS2,Close
Set objRS2 = Nothing
objRS1.Close
Set objRS1 = Nothing
objADO.Close
Set objADO = Nothing
'*
'* Finish Message
'*
MsgBox intSQL & " EXECs",vbInformation,cVBS

Sub Process(Computer)
'****
'* Process each Computer
'****
If Computer = "" Then Exit Sub
On Error Resume Next
'*
'* Declare Variables
'*
Dim objITM
Dim objQRY
Dim objWMI
'*
'* WMIService
'*
Set objWMI = GetObject("winmgmts://" & Computer)
If Err.Number <> 0 Then
Call Results(Computer,Err.Number,Err.Description,"")
wscript.echo "Start Error: " & err.number & " " & err.description
Else
'*
'* HotFix Information
'*
strSQL = "SELECT * FROM Win32_QuickFixEngineering"
Set objQRY = objWMI.ExecQuery(strSQL,,48)
For Each objITM In objQRY
strSQL = "EXEC SysMon_Win32_QuickFixEngineering '" _
& Computer & "','" _
& objITM.HotFixID & "','" _
& objITM.Description & "','" _
& objITM.InstalledBy & "','" _
& objITM.InstalledOn & "'"
Call
Results(Computer,Err.Number,Err.Description,objITM.HotFixID)
wscript.echo "HotFix Information SQL: " & strSQL ' Debugging
Next
'*
'* Memory Information
 
 
 

Coding Practices, Best methods?

Post by Matthe » Thu, 21 Sep 2006 23:17:38

Found a bug, Not with the program per say. during the hot fix check, I
am collecting information of who installed the fix, in some cases it
comes across a name that is has a hyphen in it such as O'Riley the
program does not bomb out because the On Error Resume Next. But is the
a simple way to fix this type of bug?

Thanks

-Matt-


Matthew wrote:

 
 
 

Coding Practices, Best methods?

Post by McKiraha » Thu, 21 Sep 2006 23:36:02


You mean apostrophe, a single quotation mark; (not hyphen "-").

Try changing this line:
& objITM.Description & "','" _
to this:
& Replace(objITM.Description,"'","`") & "','" _

It changes any apostrophes (if present) to a back quotation mark.
Thus, "O'Reilly" will become "O`Reilly".

Alternatively, all current uses of apostrophe could be changed
to use double quotation marks via Chr(34) -- a lot more work.
 
 
 

Coding Practices, Best methods?

Post by Me here : » Thu, 21 Sep 2006 23:50:45


Why not
Replace(objITM.Description,"'","''") & "','" _

which is two single quotes instead of one which is sent as one (for some
weird reason)

Always worked for me.

I use this one every textual input which is going into an sql query just in
case someone enters an apostrophe.

Angie
 
 
 

Coding Practices, Best methods?

Post by Matthe » Fri, 22 Sep 2006 00:52:59

In this case it will not work for a SQL injection

In SQL when running a stored procedure it uses only single quotes to
delimit strings.

Exec SomeFunction "Insert Some Text"
will not work
Exec SomeFunction 'Insert Some Text'
does

@McKirahan; It worked perfectly.
 
 
 

Coding Practices, Best methods?

Post by Matthe » Fri, 22 Sep 2006 02:13:21

@McKirahan
Can you help me with this section. I have it working as a separate
process using wscript.echo in another vbs scrip, but I am having
trouble getting it to work inline.

BTW Using your system as a template I successfully added to more
modules.

Thanks Again

-Matt-

[Code]

strSQL = "SELECT * FROM Win32_OperatingSystem"
Set objQRY = objWMI.ExecQuery(strSQL,,48)

Dim dtmInstallDate
Set dtmInstallDate = CreateObject("WbemScripting.SWbemDateTime")
dtmInstallDate.Value = strOS.InstallDate
Dim dtmLastBootUpTime
Set dtmLastBootUpTime = CreateObject("WbemScripting.SWbemDateTime")
dtmLastBootUpTime.Value = strOS.LastBootUpTime

For Each objITM In objQRY
strSQL = "EXEC SysMon_Win32_OperatingSystem '" _
& Computer & "','" _
& dtmInstallDate.GetVarDate & "','" _
& dtmLastBootUpTime.GetVarDate & "','" _
& objITM.Caption & "','" _
& objITM.ServicePackMajorVersion & "','" _
& objITM.ServicePackMinorVersion & "','" _
& objITM.Version & "'"
Call Results(Computer,Err.Number,Err.Description,"")
wscript.echo "Operating System Information SQL: " & strSQL '
Debugging
Set dtmInstallDate = Nothing
Set dtmLastBootUpTime = Nothing

[/code]
 
 
 

Coding Practices, Best methods?

Post by Richard Mu » Fri, 22 Sep 2006 02:31:20

Matthew,

Not sure what you mean. I always use this in SQL statements I run on
databases. For example:

strName = "O'Hara"
strName = Replace(strName, "'", "'')
strSQL = "SELECT * FROM MyTable WHERE UserName = '" & strName & "'"

Or

strSQL = "UPDATE Mytable SET UserName = '" & strName & "' WHERE ID = 34"

--
Richard
Microsoft MVP Scripting and ADSI
Hilltop Lab - http://www.yqcomputer.com/
 
 
 

Coding Practices, Best methods?

Post by Richard Mu » Fri, 22 Sep 2006 02:42:07

Sorry, a typo. Should be:

strName = Replace(strName, "'", "''")

--
Richard
Microsoft MVP Scripting and ADSI
Hilltop Lab - http://www.yqcomputer.com/
 
 
 

Coding Practices, Best methods?

Post by McKiraha » Fri, 22 Sep 2006 03:11:08


What kind of "trouble" -- be specific if you want assistance.

It would have been nice if you gave me code that I could test as-is.


Where do the "strOS." values come from?

For more information visit this link:

SWbemDateTime at MSDN
URL: http://www.yqcomputer.com/
wmi/swbemdatetime.asp


Also check out:

Script Center
URL: http://www.yqcomputer.com/

Scriptomatic 2.0
http://www.yqcomputer.com/

"The world-famous Scriptomatic 2.0 is the utility that writes
WMI scripts for you. Not only that, but it teaches you the
fundamental concepts behind writing WMI scripts for yourself."