[2005] ClassLibrary Call Problem

[Tarablue]

Hello All,

I'm trying to add items to a ComboBox at Run-Time. I have the following code in the ClassLibrary;

Public Class Diameters

    Public Function Diameter(ByRef ComboBox5 As System.Windows.Forms.ComboBox) As String

        ComboBox5.Items.Add("300mm")
        ComboBox5.Items.Add("350mm")
        ComboBox5.Items.Add("400mm")
        ComboBox5.Items.Add("450mm")
        ComboBox5.Items.Add("500mm")
        ComboBox5.Items.Add("560mm")
        ComboBox5.Items.Add("630mm")

    End Function
End Class

and I call this from another Form using;

        Dim AddDia As String

        AddDia = EconoAxialFan.Diameters.Diameter(ComboBox5)

The problem I have is an error Name 'ComboBox5' is not declared on ComboBox5 in this last piece of code.

Now I have used the same method to call this as I have used in other parts of my program without any problems, so why this should now happen beats me.

Best Rgds,
Tarablue

[jmcilhinney]

First up, let's take a look at that method and fix some of the issues. First up, why is that method a Function when it should be a Sub? You've declared it as a Function that returns a String, but it doesn't return anything at all so why is it Function?

Secondly, why is parameter declared ByRef when it should be ByVal? The only reason to declare a reference type parameter ByRef is if you want to assign a value to the parameter within the method and have the original variable reflect the change too. You aren't assigning anything to the parameter inside the method so there's no reason to declare it ByRef. ComboBox is a class so it's a reference type.

When you pass a reference type ByVal the parameter still refers to the same object as the original variable, so any changes you make to the object will affect the original because there is only one object. The only change that will NOT be reflected in the original is assigning a whole new object to the parameter, which you're not doing.

Now let's take a look at your abominable naming. Why would you call the parameter "ComboBox5"? It's one thing to accept the default names for controls when you add them to a form and end up with a ComboBox5 that way but to create one yourself... Where are ComboBoxes 1 to 4 that there should be any reason to use a name like "ComboBox5"? You should use a sensible name. I would use "target" or "comboBox" as my first two choices.

Next is the name of your method. Method names should describe what the method does. Does the name "Diameter" do that? No, it doesn't. "Diameter" would be a good name for a property that returned the diameter of a circle. Your method populates a ComboBox with some measurements, so that's what the name should indicate. There's nothing inherent in that method that says that those values are diameters, so the method name shouldn't imply that there is. You could just as easily use that method with a ComboBox that displayed the length of the side of a square, or many other things. The purpose of that method is to populate a a ComboBox with some measurements so the name should reflect that. A good example would be "LoadStandardLengths" or something like that.

Now, let's get to the actual question. You've declared a parameter named ComboBox5 in your method so that error message won't be appearing there. It will be appearing on the line that calls the method:

AddDia = EconoAxialFan.Diameters.Diameter(ComboBox50

ComboBox5 there does NOT refer to the ComboBox5 parameter in the method. It refers to a variable or property in the scope of that call. Is there a variable or property wihtin the scope of that call named ComboBox5? The error message suggests not.

Now, having said all that, that method is inefficient and, on top of that, completely unnecessary. The inefficiency comes from the fact that you should not add items to a ComboBox one by one like that. If you have multiple items to add then you should do it in a batch:

ComboBox5.Items.AddRange(New String() {"300mm", "350mm", "400mm", "450mm", "500mm", "560mm", "630mm"})

Now, the fact that you can load all the items into the ComboBox in one method call means that you don't need that method at all. You simply define a variable or property that returns a String array conraining those values:

Public ReadOnly Property Diameters() As String()
    Get
        Return New String() {"300mm", "350mm", "400mm", "450mm", "500mm", "560mm", "630mm"}
    End Get
End Property

Then, wherever you want to populate a ComboBox, instead of calling that current method you simply call Items.AddRange and pass that variable or property:

myComboBox.Items.AddRange(Diameters)

[Tarablue]

Hello jmcilhinney,

OK, first of all let me say that all your comments and advice have been well received, accepted and taken on board. Obviously one gets a little bit wounded when you believe your code is OK, but if you cannot take constructive criticisms about your work then you will not go far - thank you.

As for the rest, what I'm embarking upon now is all very new to me and I still have a huge amount of stuff to read, learn and absorb about VB.net so it will take me a while to get a good handle on what you have offered.

In closing, your last comment to me on the infamous GoTo statement, I have now dropped it from my current code, so I'm getting slowly weaned off GWBasic.

Once again - Thank You!!

Best Rgds,
Tarablue

[jmcilhinney]

There's a lot to learn, no doubt. One point to note with naming conventions is that, even if you know what you're doing now, going back to code later when the names are arbitrary is a good way to confuse yourself. It's quite surprising how quickly we forget what our intentions were. Even if you can see what is doing, it's often hard to work out why. Sensible naming conventions will help there. It will also help those who have never seen your code before, like those who try to help you on forums.