Skip to main content
Code Review

Return to Answer

added 508 characters in body
Source Link
RubberDuck
RubberDuck
  • 31.2k
  • 6
  • 74
  • 177

  • I like your Queries class. A lot. I do question whether it actually needs to be a class though. It seems that a standard module would work fine, but perhaps you're doing this to hide the functions from Excel's formula bar?? If that's the case, I like it even more.

  • I'm not saying it's necessarily better, but I think maybe Part could a Type instead of a Class. It doesn't really do anything. It's just a collection of values, which is what Types are for. Just something to ponder on.

  • I like your Queries class. A lot. I do question whether it actually needs to be a class though. It seems that a standard module would work fine, but perhaps you're doing this to hide the functions from Excel's formula bar?? If that's the case, I like it even more.

  • I'm not saying it's necessarily better, but I think maybe Part could a Type instead of a Class. It doesn't really do anything. It's just a collection of values, which is what Types are for. Just something to ponder on.

Source Link
RubberDuck
RubberDuck
  • 31.2k
  • 6
  • 74
  • 177

  • Bitwise conditionals make no sense to the "average" VBA dev. I like that you left a comment here, but consider leaving an remark that the check is done bitwise.
' if sucessfully connected then
If (cn.State And adStateOpen) = adStateOpen Then
  • I know you asked us not to bash on your use of c for collection, and I honestly don't mind it in your custom Parts collection class, but I really don't like your use of it here in Module1.
    Dim c As Parts
    Set c = New Parts

    BuildTheCollection c

    If Not IsEmpty([A1]) Then Cells.Delete ' clear spreadsheet

    PrintTheCollection c, 1 ' being called resursively

Have you ever tried doing a Ctl+H to replace a single letter variable name? (Hint: Don't hit "replace all" when doing so.)

  • PrintTheCollection could have a better name, but I'm more concerned that you have to pass it a 1 here in your Main routine. I would make the argument optional and default to one. It makes it a little cleaner and removes the need for the comment here.

  • Are you sure you're cleaning up as you intend to?

If Not (cn Is Nothing) Then
    If (cn.State And adStateOpen) = adStateOpen Then
        cn.Close
        Set cn = Nothing
    End If
    Set cnWrapper = Nothing
End If

I would think that you would want to set the connection to Nothing whether or not it was open. Also, calling .Close on an already closed connection does no harm, so I'm not real sure why you're checking it's adState. I feel like this would be simpler.

    If Not (cn Is Nothing) Then
        cn.Close
        Set cn = Nothing

        Set cnWrapper = Nothing
    End If

  • This code also seems to show up a lot in what you've shown us here. So, first dry it up by writing a subroutine to take care of the clean up.

  • Actually, this code shows up a lot in Error Handlers. Would it be simpler to just let the error bubble up and handle the clean up from your Sub Main? I would consider it. I feel like you've left a lot of places where the global connection could get closed, but then the code just keeps chugging along like it still has a valid connection.

  • What is p.T again??? A part property of some kind or other. ;)

p.T = rs(1)