Question Breaking down a nested mess

sentinel0

Member
Joined
Apr 18, 2012
Messages
14
Location
Cincinnati, OH
Programming Experience
1-3
I have a project that I started and initially it was not to complicated parse a spreadsheet then run a command on each item parsed. It has since grown into a really nasty mess because of my lack of true programming skills. I have a button that has ~180 lines of code in it. Most of the code flows like this if this is true and this is true and this is true allow this to happen. I have a lot more if's in there and I need to add some additional functionality to that button now but, I know the first step to that is to get some control of my flow control some how break it down it put it into a class by it's self. I got so many lines now I just don't know where to begin. Not to mention when I started writing this I was writing it in vbscript/hta and have slowly converted it over to .net. I'm going to post my production button code and my test code that is more .net friendly. With the hopes so one can please help me break down some of the flow control into something not so blah.

Production:
VB.NET:
        Const ForWriting = 2 
        Dim objFso, objExcel
        Dim booleanVal As Boolean
        Dim intCount As Integer
        Dim intRow As Integer
        Dim dtmStartTime As DateTime
        Dim strDUser As String
        Dim strDPassword As String
        Dim strNewCname As String
        Dim strOldCname As String

        Dim now As DateTime = DateTime.Now
        dtmStartTime = CType(now.ToString("G"), DateTime)
        objExcel = CreateObject("Excel.Application")
        objFso = CreateObject("Scripting.FileSystemObject")

        booleanVal = True

        strDUser = txtUserName.Text
        strDPassword = txtPassword.Text
        If mfl.ValidateCredentials(strDUser, strDPassword) = False Then
            booleanVal = False
            MsgBox("Incorrect Username or password entered!")
            WriteLog("NetDomTool Rename Report:")
            WriteLog("Runtime: " & dtmStartTime)
            WriteLog("Error: Incorrect Username or password entered!")
            WriteLog("Terminating application!")
            WriteLog("CloseWriter")
            Application.Exit()
        Else
        End If
        If booleanVal = True Then
            WriteLog("NetDomTool Rename Report:")
            WriteLog("Runtime: " & dtmStartTime)
            If StrFile = "" Then
                booleanVal = False
                MsgBox("No File Selected!")
                WriteLog("Error: File passed to NetDomTool - Null")
                WriteLog("Terminating application!")
                WriteLog("CloseWriter")
                Application.Exit()
            Else
                Dim intRecordCount As Integer = mfl.RecordCount(StrFile)
                ProgressBar1.Maximum = intRecordCount
                ProgressBar1.Step = 1
                WriteLog("File passed to NetDomTool - " & StrFile)
                WriteLog("Total items to process: " & intRecordCount & vbCrLf)
            End If
        Else
        End If
        'Allows processing if username and password are entered
        If booleanVal = True Then
            objExcel.WorkBooks.Open(StrFile)

            intCount = 0
            intRow = 2

            Do Until objExcel.Cells(intRow, 1).Value = ""
                booleanVal = True
                On Error Resume Next
                strOldCname = objExcel.Cells(intRow, 1).Value
                strNewCname = objExcel.Cells(intRow, 2).Value
                'Checks if new and old computer accounts exist before proceeding
                If mfl.ValidateComputer(strOldCname) = True Then
                    WriteLog("PC to be renamed: " & strOldCname & " Found in AD!")

                    'Checks whether old machine is connectable
                    Dim objSWbemLocator, objSWbemServices

                    objSWbemLocator = CreateObject("WbemScripting.SWbemLocator")
                    objSWbemServices = objSWbemLocator.ConnectServer(strOldCname, "root\cimv2")
                    If Err.Number <> 0 Then
                        objExcel.Cells(intRow, 3).Value = ("PC to be renamed: " & strOldCname & " unable to communicate!")
                        DataGridView1.Rows.Add(strOldCname, strNewCname, objExcel.Cells(intRow, 3).Value)
                        WriteLog("Error: PC to be renamed: " & strOldCname & " unable to communicate!")
                        WriteLog("Skipping Rename!" & vbCrLf)
                        Err.Clear()
                        intCount = intCount + 1
                        booleanVal = False
                    End If

                    'Checks if the new computer account already exists and skips if true
                    If mfl.ValidateComputer(strNewCname) = True Then
                        objExcel.Cells(intRow, 3).Value = ("New PC Name: " & strNewCname & " already exists!")
                        DataGridView1.Rows.Add(strOldCname, strNewCname, objExcel.Cells(intRow, 3).Value)
                        WriteLog("Error: New PC Name: " & strNewCname & " already exists!")
                        WriteLog("Skipping Rename!")
                        booleanVal = False
                        intCount = intCount + 1
                    End If


                    'Checks if old computer account exists and skips if false    
                ElseIf mfl.ValidateComputer(strOldCname) = False Then
                    booleanVal = False
                    objExcel.Cells(intRow, 3).Value = ("PC to be renamed: " & strOldCname & " Not Found in Domain!")
                    DataGridView1.Rows.Add(strOldCname, strNewCname, objExcel.Cells(intRow, 3).Value)
                    'InvalidPCArr = InvalidPCArr & objExcel.Cells(intRow, 1).Value & vbcrlf
                    WriteLog("Error: PC to be renamed: " & strOldCname & " Not Found in Domain!")
                    WriteLog("Skipping Rename!" & vbCrLf)
                    intCount = intCount + 1
                End If

                'Lets script proceed if it has made it through all conditionals
                If booleanVal = True Then
                    'DataGridView1.Rows.Add(strOldCname, strNewCname)
                    RegTatoo(strOldCname, strDUser, strNewCname)

                    If objExcel.Cells(intRow, 4).Value = 1 Then
                        RegAutoLogin(strNewCname, strOldCname)
                        DataGridView1.Rows.Add(strOldCname, strNewCname, "", objExcel.Cells(intRow, 4).Value)
                    Else
                        DataGridView1.Rows.Add(strOldCname, strNewCname)
                    End If
                    'Builds NETDOM statement and executes
                    Dim myProcess As New Process()
                    myProcess.StartInfo.FileName = "NetDom.exe"
                    myProcess.StartInfo.UseShellExecute = False
                    myProcess.StartInfo.RedirectStandardOutput = True

                    Dim strCommand As String
                    'Dim strCmdOutput As String
                    'Dim oExec
                    If ckbForceReboot.Checked Then
                        myProcess.StartInfo.Arguments = "RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force /reboot:60"
                        strCommand = "NETDOM RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force /reboot:60"
                        WriteLog("Netdom command being executed:")
                        WriteLog(Replace(strCommand, strDPassword, "********"))
                    ElseIf ckbSchedReboot.Checked Then
                        ' Schedules the system reboot in the PC's Task Scheduler
                        If Scheduler.Execute(strOldCname, DateTimePicker1.Value) = True Then
                            WriteLog("Success: Reboot has been scheduled for PC: " & strOldCname & " at: " & DateTimePicker1.Value().ToString)
                        Else
                            WriteLog("Error: Failed to schedule system reboot")
                        End If
                        myProcess.StartInfo.Arguments = "RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force"
                        strCommand = "NETDOM RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force"
                        WriteLog("Netdom command being executed:")
                        WriteLog(Replace(strCommand, strDPassword, "********"))
                    Else
                        myProcess.StartInfo.Arguments = "RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force"
                        strCommand = "NETDOM RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force"
                        WriteLog("Netdom command being executed:")
                        WriteLog(Replace(strCommand, strDPassword, "********"))
                    End If
                    myProcess.Start()
                    WriteLog(myProcess.StandardOutput.ReadToEnd())
                    Console.WriteLine(myProcess.StandardOutput.ReadToEnd())
                    myProcess.WaitForExit()
                End If
                intRow = intRow + 1
                DataGridView1.EndEdit()
                DataGridView1.Refresh()
                DataGridView1.Parent.Refresh()
                ProgressBar1.PerformStep()
            Loop


            MsgBox("NetDomTool has completed!")
            WriteLog("NetDomTool has completed!" & vbCrLf)


            'Saves errors occured in processing to Excel file 
            objExcel.ActiveWorkbook.Save()


            If intCount <> 0 Then
                MsgBox("There where " & intCount & " errors" & vbCrLf & "Please review the error log at " & StrPath & "PCRename_ERR_LOG-" & DtBuild & ".log")
                WriteLog("Error: Total errors in log " & intCount)
            Else
                MsgBox("There where " & intCount & " errors")
                WriteLog("Success: Total errors in log " & intCount)
            End If
            WriteLog("CloseWriter")
            StrFile = ""
            'Closes spreadsheet and terminates Excel process
            objExcel.ActiveWorkbook.Close()
            objExcel.Application.Quit()
            objExcel = Nothing
        ElseIf booleanVal = False Then
            MsgBox("Please correct information!")
        End If

Testing:
VB.NET:
Const ForWriting = 2 
       Dim booleanVal As Boolean
        Dim intCount As Integer
        Dim intRow As Integer
        Dim dtmStartTime As DateTime
        Dim strDUser As String
        Dim strDPassword As String
        Dim strNewCname As String
        Dim strOldCname As String
        Dim range As Excel.Range
        range = ExWs.UsedRange
        Dim rCount As Integer
        Dim now As DateTime = DateTime.Now
        dtmStartTime = CType(now.ToString("G"), DateTime)
        booleanVal = True

        strDUser = txtUserName.Text
        strDPassword = txtPassword.Text
        If mfl.ValidateCredentials(strDUser, strDPassword) = False Then
            booleanVal = False
            MsgBox("Incorrect Username or password entered!")
            WriteLog("NetDomTool Rename Report:")
            WriteLog("Runtime: " & dtmStartTime)
            WriteLog("Error: Incorrect Username or password entered!")
            WriteLog("Terminating application!")
            WriteLog("CloseWriter")
            Application.Exit()
        Else
        End If
        If booleanVal = True Then
            WriteLog("NetDomTool Rename Report:")
            WriteLog("Runtime: " & dtmStartTime)
            If StrFile = "" Then
                booleanVal = False
                MsgBox("No File Selected!")
                WriteLog("Error: File passed to NetDomTool - Null")
                WriteLog("Terminating application!")
                WriteLog("CloseWriter")
                Application.Exit()
            Else

                ProgressBar1.Maximum = intRecordCount
                ProgressBar1.Step = 1
                WriteLog("File passed to NetDomTool - " & StrFile)
                WriteLog("Total items to process: " & intRecordCount & vbCrLf)
            End If

        Else

        End If

        'Allows processing if username and password are entered
        If booleanVal = True Then


            intCount = 0
            intRow = 2
            
            For rCount = 2 to range.Rows.Count
                booleanVal = True
                On Error Resume Next

                strOldCname = CType(range(intRow,1),Excel.Range).Value.ToString
                strNewCname = Ctype(range(intRow, 2),Excel.Range).Value.ToString

                'Checks if new and old computer accounts exist before proceeding
                If mfl.ValidateComputer(strOldCname) = True Then
                    WriteLog("PC to be renamed: " & strOldCname & " Found in AD!")

                    'Checks whether old machine is connectable
                    If mfl.IsPingable(strOldCname) = True then
                        If mfl.IsWmiConn(strOldCname) = False then
                            CType(range(intRow, 3), Excel.Range).Value = ("PC to be renamed: " & strOldCname & " wmi failed!")
                            DataGridView1.Rows.Add(strOldCname, strNewCname, CType(range(intRow, 3), Excel.Range).Value)
                            WriteLog("Error: PC to be renamed: " & strOldCname & " wmi failed!")
                            WriteLog("Skipping Rename!" & vbCrLf)
                            intCount = intCount + 1
                            booleanVal = False
                        End If
                    Else
                        CType(range(intRow, 3), Excel.Range).Value = ("PC to be renamed: " & strOldCname & " unable to ping!")
                        DataGridView1.Rows.Add(strOldCname, strNewCname, CType(range(intRow, 3), Excel.Range).Value)
                        WriteLog("Error: PC to be renamed: " & strOldCname & " unable to ping!")
                        WriteLog("Skipping Rename!" & vbCrLf)
                        intCount = intCount + 1
                        booleanVal = False
                    End if

                    'Checks if the new computer account already exists and skips if true
                    If mfl.ValidateComputer(strNewCname) = True Then
                        CType(Range(intRow, 3),Excel.Range).Value = ("New PC Name: " & strNewCname & " already exists!")
                        DataGridView1.Rows.Add(strOldCname, strNewCname, CType(Range(intRow, 3),Excel.Range).Value)
                        WriteLog("Error: New PC Name: " & strNewCname & " already exists!")
                        WriteLog("Skipping Rename!")
                        booleanVal = False
                        intCount = intCount + 1
                    End If

                    'Checks if old computer account exists and skips if false    
                ElseIf mfl.ValidateComputer(strOldCname) = False Then
                    booleanVal = False
                    CType(Range(intRow, 3),Excel.Range).Value = ("PC to be renamed: " & strOldCname & " Not Found in Domain!")
                    DataGridView1.Rows.Add(strOldCname, strNewCname, CType(Range(intRow, 3),Excel.Range).Value)
                    WriteLog("Error: PC to be renamed: " & strOldCname & " Not Found in Domain!")
                    WriteLog("Skipping Rename!" & vbCrLf)
                    intCount = intCount + 1
                End If

                'Lets script proceed if it has made it through all conditionals
                If booleanVal = True Then
                    'DataGridView1.Rows.Add(strOldCname, strNewCname)
                    RegTatoo(strOldCname, strDUser, strNewCname)

                    If CType(Range(intRow, 4),Excel.Range).Value is "1" Then
                        RegAutoLogin(strNewCname, strOldCname)
                        DataGridView1.Rows.Add(strOldCname, strNewCname, "", CType(Range(intRow, 4),Excel.Range).Value)
                    Else
                        DataGridView1.Rows.Add(strOldCname, strNewCname)
                    End If
                    'Builds NETDOM statement and executes
                    Dim myProcess As New Process()
                    myProcess.StartInfo.FileName = "NetDom.exe"
                    myProcess.StartInfo.UseShellExecute = False
                    myProcess.StartInfo.RedirectStandardOutput = True

                    Dim strCommand As String
                    'Dim strCmdOutput As String
                    'Dim oExec
                    If ckbForceReboot.Checked Then
                        myProcess.StartInfo.Arguments = "RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force /reboot:60"
                        strCommand = "NETDOM RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force /reboot:60"
                        WriteLog("Netdom command being executed:")
                        WriteLog(Replace(strCommand, strDPassword, "********"))
                    ElseIf ckbSchedReboot.Checked Then
                        ' Schedules the system reboot in the PC's Task Scheduler
                        If Scheduler.Execute(strOldCname, DateTimePicker1.Value) = True Then
                            WriteLog("Success: Reboot has been scheduled for PC: " & strOldCname & " at: " & DateTimePicker1.Value().ToString)
                        Else
                            WriteLog("Error: Failed to schedule system reboot")
                        End If
                        myProcess.StartInfo.Arguments = "RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force"
                        strCommand = "NETDOM RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force"
                        WriteLog("Netdom command being executed:")
                        WriteLog(Replace(strCommand, strDPassword, "********"))
                    Else
                        myProcess.StartInfo.Arguments = "RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force"
                        strCommand = "NETDOM RENAMECOMPUTER " & strOldCname & " /newname:" & strNewCname & " /userd:healthall.com\" & strDUser & " /passwordd:" & strDPassword & " /force"
                        WriteLog("Netdom command being executed:")
                        WriteLog(Replace(strCommand, strDPassword, "********"))
                    End If
                    myProcess.Start()
                    WriteLog(myProcess.StandardOutput.ReadToEnd())
                    Console.WriteLine(myProcess.StandardOutput.ReadToEnd())
                    myProcess.WaitForExit()
                End If
                intRow = intRow + 1
                DataGridView1.EndEdit()
                DataGridView1.Refresh()
                DataGridView1.Parent.Refresh()
                ProgressBar1.PerformStep()
            Next

            MsgBox("NetDomTool has completed!")
            WriteLog("NetDomTool has completed!" & vbCrLf)

            'Saves errors occured in processing to Excel file 
            Dim objEx As New ClientValidation
            objEx.Save()

            If intCount <> 0 Then
                MsgBox("There where " & intCount & " errors" & vbCrLf & "Please review the error log at " & StrPath & "PCRename_ERR_LOG-" & DtBuild & ".log")
                WriteLog("Error: Total errors in log " & intCount)
            Else
                MsgBox("There where " & intCount & " errors")
                WriteLog("Success: Total errors in log " & intCount)
            End If
            WriteLog("CloseWriter")
            StrFile = ""
            'Closes spreadsheet and terminates Excel process
            objEx.Close()
        ElseIf booleanVal = False Then
            MsgBox("Please correct information!")
        End If

I know there has to be a better way to handle the flow and break it down. Any help would be awesome. thanks
Side note: The test code works however I can't figure out how to save and close the instance of Excel that I spawn from the class that creates it.
 
Okay I printed out my code and realized that I no longer need to check in the button to verify if the filepath string has a value so that code looks like this now.
VB.NET:
If booleanVal = True Then
            WriteLog("NetDomTool Rename Report:")
            WriteLog("Runtime: " & dtmStartTime)
            Dim intRecordCount As Integer = mfl.RecordCount(StrFile)
            ProgressBar1.Maximum = intRecordCount
            ProgressBar1.Step = 1
            WriteLog("File passed to NetDomTool - " & StrFile)
            WriteLog("Total items to process: " & intRecordCount & vbCrLf)
        End If

That validation is now being handled in the button that opens the file dialog.
 
Okay after staring at my code puzzled for some time I think I came up with a way to clean it up some. In my Class I have a function to validate credentials that returns bool. So for example I create a second function called ValCredLog(byval ValCred(user,pw)) and then return bool back to main form. I know I validating 3 things Authentication of user, Computer object exist, and isConnectable. I just need to break it down into smaller pieces.
 
It would probably help you out to firstly draw a functional flowchart of the program. From there you can just rewrite the control loop and paste in/modify the code that each branch executes. There is no single right answer to your question, what you consider a "better" approach at flow control might be better to read, but not necessarily better in performances.

From the looks of it this doesn't look too bad really... THIS is bad:

  ' Clear empty rows and unwrap multi-line Descriptions
  currentRow = startRow 
  Do While currentRow =< endRow
    currentCol = startCol : rowMarker = False
    Do While currentCol =< endCol
      If CStr(objSheet.Cells(currentRow, currentCol).Value) <> "" Then
        If currentCol = startCol And currentRow > startRow And UseRegEx then
            If UseTextNotValue = True Then
                CellText = objSheet.Cells(currentRow, currentCol).Text
                CellText2 = objSheet.Cells(currentRow - 1, currentCol).Text
            Else
                CellText = objSheet.Cells(currentRow, currentCol).Value
                CellText2 = objSheet.Cells(currentRow - 1, currentCol).Value
            End If
            If Not CellRegExp.Test(CellText) then
                objSheet.Cells(currentRow - 1, currentCol).Value = CellText2 & " " & CellText
                rowMarker = False
            End If
        Else
          rowMarker = True
        End If                                           
      End If
      currentCol = currentCol + 1
    Loop
    If Not rowMarker then
      objSheet.Cells(currentRow, currentCol).EntireRow.Delete
      endRow = endRow - 1
    Else
      currentRow = currentRow + 1
    End If
  Loop

But in this case, necessary...
 
Thank you for the reply, It just looks so messy with all those lines under a button sub. I'm trying to think of a way to consolidate the validation logging into the class that handles the validation functions just to keep it clean. If anyone happens to read this I have an additonal question what is a reasonable ammount of classes in a single form project. As I code more I'm getting better and not lumping stuff together and breaking into specialized classes but, I want to make sure I'm not going overboard. What is the best practice on what you should do with the class object created after it has been used in a sub? Should I dispose, set to nothing, or not worry about it?
 
Usually just break it down in as many parts as is useful. I normally break things down as modular as possible. Whenever I write a set of methods to solve a new problem I always write the code thinking it will be used elsewhere eventually, as it almost always does. Over the years you will build yourself quite a nice set of classes for many higher-level tasks, like logging, interacting with databases, printing, generating reports, etc... You then add whichever of these classes you need to a project, and if there are too many standalone classes, you can just compile them together in a helper library for your project. Eventually writing an app will be just a series of conditional blocks and calls to your library.

For example, re-reading your original post:

            MsgBox("Incorrect Username or password entered!")
            WriteLog("NetDomTool Rename Report:")
            WriteLog("Runtime: " & dtmStartTime)
            WriteLog("Error: Incorrect Username or password entered!")
            WriteLog("Terminating application!")
            WriteLog("CloseWriter")
            Application.Exit()


Could be put into "Sub BadLoginWithExit(ByVal dtmStartTime As DateTime)" in the Log class, and then all you have left in the form is:

        If mfl.ValidateCredentials(strDUser, strDPassword) = False Then Log.BadLoginWithExit(dtmStartTime)
 
Last edited:
Herman,
Thank you that is genius and I will certainly impliment that approach in my code. I have ~2 projects I'm working on. One that matters and one that's just for play. The one that matters is just getting to the point of being very hard to manage. Once I learned the power of classes I kind of went crazy so now I've got to go through all the code add/remove functions and classes that I once thought was a good idea, lol. I've been self teaching myself .Net and it is tough stuff, it's been over 10 years since I've had any schooling in regards to programming in VB and I think we were only at VB6 at the time and it seemed so much simpler than the .Net that came out shortly after. It's coming around though. Thanks again.
 
Back
Top