Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Does the following code look okay? Want to see if anyone can spot any mistakes I may have made. This will be pulling from a pre-formatted Excel document - I have already specified all needed Cell locations.

Things I want clarified:

  • How many times can you invoke SetInfo and did I use it more than needed? I was under the impression that a SetInfo would be needed before a SetPassword can be used, but I could be very wrong.

  • Does this line look okay? Was I correct to separate multiple cells using a comma in order to put all the info together? I know it worked with an Echo command:

    Set objUser = objOU.Create _
    ("User", "cn=" & objExcel.Cells(intRow, 3).Value, objExcel.Cells(intRow, 2).Value, objExcel.Cells(intRow, 19).Value)
    
  • Is this the correct code for setting a password to never expire?

    objUser.Put "userAccountControl", intUAC XOR _
    ADS_UF_DONT_EXPIRE_PASSWD
    
  • Full code below, I'd appreciate any feedback on anything else that is found.

    Set objExcel = CreateObject("Excel.Application")
    Set objWorkbook = objExcel.Workbooks.Open _
        ("C:\Book1.xls")
    intRow = 2
    Do Until objExcel.Cells(intRow,1).Value = ""
        Set objOU = GetObject _
        ("ou=" & objExcel.Cells(intRow, 13).Value & _
            ", dc=satdc, dc=com")
        Set objUser = objOU.Create _
            ("User", "cn=" & objExcel.Cells(intRow, 3).Value, objExcel.Cells(intRow, 2).Value, objExcel.Cells(intRow, 19).Value)
        objUser.sAMAccountName = objExcel.Cells(intRow, 19).Value & ".sat"
        objUser.GivenName = objExcel.Cells(intRow, 3).Value
        objUser.SN = objExcel.Cells(intRow, 2).Value
        objUser.SetInfo
        objUser.AccountDisabled = False
        objUser.AccountExpirationDate = Date + 365
        objUser.SetPassword objExcel.Cells(intRow, 9).Value
        objUser.SetInfo
        objUser.Put "userAccountControl", intUAC XOR _
            ADS_UF_DONT_EXPIRE_PASSWD
        objUser.HomeDirectory = "\\satdc" & "\" & "Users" & "\" & _
            objUser.Get("sAMAccountName")
        objUser.homeDrive = "U:"
        objUser.SetInfo
        intRow = intRow + 1
    Loop
    objExcel.Quit
    
share|improve this question
add comment

1 Answer

Naming: Hungarian Notation is the DEVIL.

It makes the reader want to see your code burn in eternal flames.

enter image description here

http://simpsonswiki.com/wiki/Flanders_the_Devil

  • objExcel would be better off as xlApp or excelApp
  • objUser is dying to call itself something like adUser
  • intRow wants to be called xlRow or just row
  • intUAC is silently begging for a more descriptive name
  • objOU wants to shoot whoever named it that

Now, whether your code is correct or not is off-topic for this site; since this question hasn't been closed as off-topic yet, I'm going to assume you've got working code.

Do Until objExcel.Cells(intRow,1).Value = ""

I'd use vbNullString here instead of "". It makes the intent clearer, and the non-string takes up 0 bytes. "" eats 6 bytes for no reason - StrPtr(vbNullString) is 0; StrPtr("") is a non-zero memory address.

The repeated assignments on objUser are a missed opportunity of using a With block:

With objOU.Create("User", "cn=" & objExcel.Cells(intRow, 3).Value, _
                          objExcel.Cells(intRow, 2).Value, _
                          objExcel.Cells(intRow, 19).Value)

    .SAMAccountName = objExcel.Cells(intRow, 19).Value & ".sat"
    .GivenName = objExcel.Cells(intRow, 3).Value
    .SN = objExcel.Cells(intRow, 2).Value
    .AccountDisabled = False
    .AccountExpirationDate = Date + 365
    '...

End With

Your usage of line continuation characters is... well, you use it in weird places. Actually, everywhere you've used it, I wouldn't have. Notice in the above snippet, how I used it to line up the parameters.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.