What is the correct way to call a method from another class?

Question

public partial class Form1 : Form { private void StartApp() { LobGamma.LogInPanel.FillComboBox(LogInpanel_ComboBox, LobGamma.Connection.ObtainConnection()); } } public class LogInPanel { public static void FillComboBox(ComboBox Box, SqlConnection con) { Box.Items.Clear(); using (con) { SqlCommand com = new SqlCommand("Select Id From UsersTable", con); con.Open(); using (SqlDataReader reader = com.ExecuteReader()) { while (reader.Read()) { Box.Items.Add(reader["Id"].ToString()); } reader.Close(); } } con.Close(); }

I want to know if I am going the right way about using a method from another class. I only need to use the method once. Is the way I have done it by using a static method acceptable? or should the method be non-static.

If the method should be non-static, Is it best to Inherit from IDisposable so that I can use the Class in a Using Statement? or would simply creating an instance of the class and waiting for GC be acceptable?

This is not really answering your question but I think you should not use using on an SqlConnection which has been passed to the method as a parameter. This may not be a good idea, as you are disposing an instance you have been given from outside the method. This may surprise the caller. Consider moving the using to the StartApp method or creating the SqlConnectioninside FillComboBox. — wkl
– wkl, Commented Jan 2, 2017 at 13:48
static/instance doesn't really matter here. Your code is OK-ish but it still mixes GUI and business logic. Consider a method that doesn't know about Comoboxes but returns a List<string> — Henk Holterman
– Henk Holterman, Commented Jan 2, 2017 at 13:53
Thanks, does it not matter because its not really doing much? Thanks for the advice about mixing in GUI, I'll sort that out. — Jones
– Jones, Commented Jan 2, 2017 at 13:54

Sweeper · Accepted Answer · 2017-01-02 13:58:51Z

In my opinion, FillComboBox should be in the form class as a private non-static method:

public partial class Form1 : Form { private void StartApp() { LobGamma.LogInPanel.FillComboBox(LogInpanel_ComboBox, LobGamma.Connection.ObtainConnection()); } private void FillComboBox(ComboBox Box, SqlConnection con) { Box.Items.Clear(); using (con) { SqlCommand com = new SqlCommand("Select Id From UsersTable", con); con.Open(); using (SqlDataReader reader = com.ExecuteReader()) { while (reader.Read()) { Box.Items.Add(reader["Id"].ToString()); } reader.Close(); } } con.Close(); } }

This is because all FillComboBox does is figure out what contents should be in the combo box. That's related to the UI so why not put it in the form class? The form class is supposed to initialize UI components and UI related things, which is exactly what FillComboBox is doing.

Is it best to Inherit from IDisposable so that I can use the Class in a Using Statement?

You only need to implement IDisposable if there is something to be disposed. But since you already have a using statement inside FillComboBox, everything is disposed already!

Alternatively, don't pass the combo box at all:

 private void FillComboBox(ComboBox Box, SqlConnection con) { LogInpanel_ComboBox.Items.Clear(); using (con) { SqlCommand com = new SqlCommand("Select Id From UsersTable", con); con.Open(); using (SqlDataReader reader = com.ExecuteReader()) { while (reader.Read()) { LogInpanel_ComboBox.Items.Add(reader["Id"].ToString()); } reader.Close(); } } con.Close(); }

i was about to write the same answer basically, so I just want to point out you don't have to pass the combobox this way because its a member of the class and can be retrieved using this.LogInpanel_ComboBox which is more readable to (you know where it lives so to say)
@nozzleman I thougt the OP might have other combo boxes that wants to be filled though. But yeah, that is a good idea. I'll edit it
also, if not already done that way: this type of behaviour is usually triggered by an eventhandler which is attached to the Forms Load-Event
Yes, FillComboBox() belongs to the Form itself. But all that SqlDataReader stuff doesn't.

Trickzter · Accepted Answer · 2017-01-02 13:57:41Z

The question is if you do really need a class for that operation. Filling an object's item list on construction can be easily done by one method called by the constructor and does not need to be a class. However, if you are building up something big like a dataContext handling the connection between your business and your data layer (a database e.g.), then you should be using a normal class (or even better using something like Entity Framework).

For tiny CRUD operations simply use a private method.

Collectives™ on Stack Overflow

What is the correct way to call a method from another class?

2 Answers 2

5 Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

5 Comments

Comments

Related