CheckedListBox Problems

[bjwade62]

I have two problems with my code for a CheckedListBox. I have a sub that checks to see if an item in a CheckedListBox is checked. If it is it's supposed to copy the corrisponding file to a folder. See code below.

 

Problem #1

I get an IndexOutOfRangeException error at line:

clbText = CheckedListBox1.CheckedItems.Item(i)

 

Problem # 2

After the file is copied I want the checked item to become unchecked.

 

Here's my code. Thanks.

 

       If Me.CheckedListBox1.CheckedItems.Count > 1 Then
           Dim clbText As String
           Dim i As Integer
           For i = 0 To CheckedListBox1.Items.Count - 1
               If Me.CheckedListBox1.GetItemCheckState(i) Then
                   clbText = CheckedListBox1.CheckedItems.Item(i)
                   'copies selected file to project folder
                   Dim strFileToSaveNoPath As String = System.IO.Path.GetFileNameWithoutExtension(clbText)
                   Dim strFileToSaveNoPathDWG As String = strFileToSaveNoPath & ".dwg"
                   strFileToSave = Me.dirStdDtl.Path & "\" & strFileToSaveNoPath & ".dwg"
                   Dim strNewFile As String = Me.proj_dir.Path & "\" & strFileToSaveNoPathDWG
                   Dim FileExists As Boolean = System.IO.File.Exists(strNewFile)

                   If FileExists = True Then
                       Dim msg As String = ("One or more detail(s) already exist in " & Me.proj_dir.Path & "." _
                              & vbCrLf & "Are you sure you want overwrite existing detail(s)?")
                       Dim Response As String = MsgBox(msg, MsgBoxStyle.OkCancel, "File(s) already exists")
                       Select Case Response
                           Case vbOK
                               IO.File.Delete(strNewFile)
                               System.IO.File.Copy(strFileToSave, strNewFile)
                           Case vbCancel
                               Exit Sub
                       End Select
                   Else
                       System.IO.File.Copy(strFileToSave, strNewFile)
                   End If
               End If

               Me.CheckedListBox1.SetItemCheckState(i, CheckState.Unchecked)
               Me.CheckedListBox1.Refresh()
           Next i
       End If

[snarfblam]

I'm guessing that you are switching over from VB6. For starters, I recommend you keep option strict on. It will catch certain errors before you even compile. For instance, for your fifth line you are implicitly converting a CheckState value into a boolean which could result in undesirable behavior (in fact, you have this kind of problem in a few places in your code).

 

The reason you are getting a bounds error is because on the 6th line you are accessing items in CheckedListBox1.CheckedItems using an index based on all the items. When you get to item #3 you looking at CheckedItems(2) when you should be looking it Items(2).

 

' This variable tracks if the user has okayed overwriting old files so we only need to ask once.
Dim Overwrite As Boolean = False

If Me.CheckedListBox1.CheckedItems.Count > 1 Then
   For i As Integer = 0 To CheckedListBox1.Items.Count - 1
       ' You should explicitly compare the CheckState value to what you expect
       If Me.CheckedListBox1.GetItemCheckState(i) = CheckState.Checked Then
           ' Try to use clear meaningful variable names. When you are using a strongly typed
           '    language with intellisense there is very little need for hungarian naming conventions
           '    (i.e. starting variable names with "str," "int," etc.
           ' ListBoxes store items as System.Objects. You should convert them back to strings.
           ' Since the looping variable (i) is based on Items.Count and not CheckedItems.Count, you should be accessing items
           '    using the Items(i) property instead of the CheckedItems(i) property.
           Dim OldFilename As String = CheckedListBox1.Items(i).ToString()

           ' Determine new filename. Not sure exactly how you constructed the filename
           ' but you should make more use of the Path class to guaruntee file path manipulations
           ' are correct and the code is more readable.
           Dim NewFilename As String = Path.Combine( _
               Path.Combine(Me.proj_dir.Path, dirStdDtl.Path), _
               Path.GetFileNameWithoutExtension(OldFilename) & ".dwg")

           ' I rewrote this whole part of the code. It is clearer to me now but you should do what makes sense to
           ' you when it comes to the logical layout of code.
           If File.Exists(NewFilename) Then
               If (Overwrite) Then
                   ' If the user has already okayed overwriting files just delete the old one.
                   File.Delete(NewFilename)
               Else
                   ' Otherwise ask the user.
                   ' The "proper" .Net way of using message boxes is with the MessageBox class.
                   ' Also, neither MsgBox() nor MessageBox.Show() return a string. This is another
                   '     example of where option strict would help.
                   Dim Response As DialogResult = _
                       MessageBox.Show("One or more detail(s) already exist in " & Me.proj_dir.Path & "." _
                          & Environment.NewLine & "Are you sure you want overwrite existing detail(s)?", _
                          "File(s) already exist.", _
                          MessageBoxButtons.OKCancel)

                   'Based on the response, either delete old file or abort the operation
                   Select Case Response
                       Case Windows.Forms.DialogResult.OK
                           File.Delete(NewFilename)
                           Overwrite = True 'User has okayed overwrite, don't ask again
                       Case Windows.Forms.DialogResult.Cancel
                           Return ' Just FYI. Same as 'Exit Sub'
                   End Select
               End If
           End If

           ' Copy the file
           System.IO.File.Copy(OldFilename, NewFilename)
           ' Clear the check
           Me.CheckedListBox1.SetItemCheckState(i, CheckState.Unchecked)
       End If
   Next i
End If

There is a lot of criticism in there, but hopefully it helps your transition to VB .Net.

[bjwade62]

I'm such a rookie at .NET that I welcome criticism. That looks like it took a lot of your time so thanks tons. I'm at home right now but I can't wait to get to the office tomorrow and try your new code. I really appreciate the time you took to comment it too. Sure I could cut and paste your code, but unless understand what you did I've learned nothing.

 

Again many thanks to you Marble_Eater!

 

Bernie

 

I'm guessing that you are switching over from VB6. For starters, I recommend you keep option strict on. It will catch certain errors before you even compile. For instance, for your fifth line you are implicitly converting a CheckState value into a boolean which could result in undesirable behavior (in fact, you have this kind of problem in a few places in your code).

 

The reason you are getting a bounds error is because on the 6th line you are accessing items in CheckedListBox1.CheckedItems using an index based on all the items. When you get to item #3 you looking at CheckedItems(2) when you should be looking it Items(2).

 

' This variable tracks if the user has okayed overwriting old files so we only need to ask once.
Dim Overwrite As Boolean = False

If Me.CheckedListBox1.CheckedItems.Count > 1 Then
   For i As Integer = 0 To CheckedListBox1.Items.Count - 1
       ' You should explicitly compare the CheckState value to what you expect
       If Me.CheckedListBox1.GetItemCheckState(i) = CheckState.Checked Then
           ' Try to use clear meaningful variable names. When you are using a strongly typed
           '    language with intellisense there is very little need for hungarian naming conventions
           '    (i.e. starting variable names with "str," "int," etc.
           ' ListBoxes store items as System.Objects. You should convert them back to strings.
           ' Since the looping variable (i) is based on Items.Count and not CheckedItems.Count, you should be accessing items
           '    using the Items(i) property instead of the CheckedItems(i) property.
           Dim OldFilename As String = CheckedListBox1.Items(i).ToString()

           ' Determine new filename. Not sure exactly how you constructed the filename
           ' but you should make more use of the Path class to guaruntee file path manipulations
           ' are correct and the code is more readable.
           Dim NewFilename As String = Path.Combine( _
               Path.Combine(Me.proj_dir.Path, dirStdDtl.Path), _
               Path.GetFileNameWithoutExtension(OldFilename) & ".dwg")

           ' I rewrote this whole part of the code. It is clearer to me now but you should do what makes sense to
           ' you when it comes to the logical layout of code.
           If File.Exists(NewFilename) Then
               If (Overwrite) Then
                   ' If the user has already okayed overwriting files just delete the old one.
                   File.Delete(NewFilename)
               Else
                   ' Otherwise ask the user.
                   ' The "proper" .Net way of using message boxes is with the MessageBox class.
                   ' Also, neither MsgBox() nor MessageBox.Show() return a string. This is another
                   '     example of where option strict would help.
                   Dim Response As DialogResult = _
                       MessageBox.Show("One or more detail(s) already exist in " & Me.proj_dir.Path & "." _
                          & Environment.NewLine & "Are you sure you want overwrite existing detail(s)?", _
                          "File(s) already exist.", _
                          MessageBoxButtons.OKCancel)

                   'Based on the response, either delete old file or abort the operation
                   Select Case Response
                       Case Windows.Forms.DialogResult.OK
                           File.Delete(NewFilename)
                           Overwrite = True 'User has okayed overwrite, don't ask again
                       Case Windows.Forms.DialogResult.Cancel
                           Return ' Just FYI. Same as 'Exit Sub'
                   End Select
               End If
           End If

           ' Copy the file
           System.IO.File.Copy(OldFilename, NewFilename)
           ' Clear the check
           Me.CheckedListBox1.SetItemCheckState(i, CheckState.Unchecked)
       End If
   Next i
End If

There is a lot of criticism in there, but hopefully it helps your transition to VB .Net.

[bjwade62]

Hey Marble_eater!

 

I had the same question on another forum. Your answer works great, but this one has less code. I'm passing it on to you in case you weren't aware of it. The particular section that I'm talking about is the FileCopy section.

 

The UIOption.AllDialogs replaces the need for additonal code to overwrite existing files. Please don't get the worng idea. You know far more about VB.NET than I'll ever know. I'm just trying to pass on some information to someone who's helped me out in the past. Thanks again for all your help.

 

Bernie

 

 

'Yours
System.IO.File.Copy(OldFilename, NewFilename)

'Other Forum
Imports Microsoft.VisualBasic.FileIO
FileSystem.CopyFile(SourceFile, TargetFile, UIOption.AllDialogs)

[snarfblam]

Don't ever think that I'm too proud to learn something from a less experienced programmer, but I'm a little confused. What is the advantage of the second listing of code you posted? And just for the record, I gave VB up about a year ago in favor of C# because I like the syntax more and because VB seems to be separating itself from the .Net mainstream (for instance, the large number of VB-specific functions and the "My" feature). I can carry almost anything over from C# to VB, but vbCrLf and My.FileSystem just don't translate into C# so easily. Oh, and please stop cheating on us. It hurts us inside.

[bjwade62]

The second listing (FileSystem.CopyFile(SourceFile, TargetFile, UIOption.AllDialogs, UICancelOption.DoNothing)) eliminates the need to create a messagebox to handle finding out if the file already exists. It also overwrites it if it does. So I was able to remove all the code that handled that previously.

 

Thanks again.... I'll try to be monogamous in the future. :)

 

Don't ever think that I'm too proud to learn something from a less experienced programmer' date=' but I'm a little confused. What is the advantage of the second listing of code you posted? And just for the record, I gave VB up about a year ago in favor of C# because I like the syntax more and because VB seems to be separating itself from the .Net mainstream (for instance, the large number of VB-specific functions and the "My" feature). I can carry almost anything over from C# to VB, but vbCrLf and My.FileSystem just don't translate into C# so easily. Oh, and please stop cheating on us. It hurts us inside.[/quote']