2
\$\begingroup\$

Question: Is there a way to more efficiently or more readably write a loop that creates multiple types of controls?

Background

I've been trying to learn C# by converting some of my older VB.Net programs over. This is a method I use after the main form loads to populate a TableLayoutPanel with controls. Eventually looking something similar to this:

Example Image

Is there a way to more efficiently or more readably write this kind of method? It really feels like there are more repeated lines here than truly necessary.

I apologize for the messy C# code, I'm about a week into playing with it.

C# Code

    public static List<object> _labels = new List<object>();
    public static List<object> _entries = new List<object>();
    public static TableLayoutPanel Form_Grid = new TableLayoutPanel();

    private void wl_getFormat()
    {
        try
        {
            MySqlConnection MySql = new MySqlConnection("Server=localhost;Database=" + Session.Department + ";Uid=" + Session.Username + ";Pwd=Password2016;");
            MySql.Open();
            MySqlCommand MySqlCmd = new MySqlCommand("SELECT * FROM wl_format", MySql);
            MySqlDataReader Reader = MySqlCmd.ExecuteReader();

            int i = 0; 
            int Height = 255;
            while (Reader.Read())
            {
                string Name = Reader.GetString("Name"); //Get field name
                string Type = Reader.GetString("Type"); //Get field type

                Label L = new Label();

                if (Reader.GetInt32("Required") == 1) { L.Text = Name + "*"; } else { L.Text = Name; }

                L.Name = "L" + i;
                L.AutoSize = true;
                L.TextAlign = ContentAlignment.MiddleLeft;
                L.Visible = true;
                L.Font = new Font("Microsoft Sans Serif", 8);
                L.Dock = DockStyle.Fill;
                L.Margin = new Padding(0, 3, 0, 3);

                if (!_labels.Contains(L))
                {
                    _labels.Add(L);
                    L.MouseUp += new MouseEventHandler(wl_Lock);
                }

                Form_Grid.Controls.Add(L, 0, i + 1);
                Form_Grid.RowStyles[i + 1].SizeType = SizeType.AutoSize;

                if (Type == "Textbox" || Type == "Suggestbox")
                {
                    TextBox T = new TextBox();
                    Height += 26;

                    T.Name = "T" + i;
                    T.Dock = DockStyle.Fill;
                    T.TabIndex = i + 1;
                    if (Type == "Suggestbox")
                    {
                        T.AutoCompleteMode = AutoCompleteMode.SuggestAppend;
                        T.AutoCompleteSource = AutoCompleteSource.CustomSource;
                        string[] p = _options[i].Split('|');
                        foreach (string a in p)
                        {
                            T.AutoCompleteCustomSource.Add(a);
                        }
                    }
                    if (!_entries.Contains(T))
                    {
                        _entries.Add(T);
                        T.KeyPress += new KeyPressEventHandler(wl_hotKeys);
                    }
                    Form_Grid.Controls.Add(T, 1, i + 1);
                    Form_Grid.SetColumnSpan(T, 3);
                }
                else if (Type == "Dropbox")
                {
                    ComboBox C = new ComboBox();
                    Height += 26;

                    C.Name = "C" + i;
                    C.Dock = DockStyle.Fill;
                    C.TabIndex = i + 1;
                    C.AutoCompleteMode = AutoCompleteMode.Suggest;
                    C.AutoCompleteSource = AutoCompleteSource.ListItems;
                    C.FormattingEnabled = true;
                    C.Sorted = true;
                    string[] p = _options[i].Split('|');
                    foreach (string a in p)
                    {
                        C.Items.Add(a);
                    }
                    if (!_entries.Contains(C))
                    {
                        _entries.Add(C);
                        C.SelectedValueChanged += new EventHandler(wl_Ties);
                    }
                    Form_Grid.Controls.Add(C, 1, i + 1);
                    Form_Grid.SetColumnSpan(C, 3);
                }
                else if (Type == "Numberbox")
                {
                    NumericUpDown N = new NumericUpDown();
                    Height += 26;

                    N.Name = "N" + i;
                    N.Dock = DockStyle.Fill;
                    N.TabIndex = i + 1;
                    N.BorderStyle = BorderStyle.FixedSingle;
                    if (!_entries.Contains(N))
                    {
                        _entries.Add(N);
                    }
                    Form_Grid.Controls.Add(N, 1, i + 1);
                    Form_Grid.SetColumnSpan(N, 3);
                }
                else
                {
                    Label E = new Label();
                    Height += 26;
                    E.Name = "E" + i;
                    E.Text = "No type information found";
                    E.AutoSize = true;
                    E.TextAlign = ContentAlignment.MiddleLeft;
                    E.Visible = true;
                    E.Font = new Font("Microsoft Sans Serif", 8);
                    E.Dock = DockStyle.Fill;
                    E.Margin = new Padding(0, 3, 0, 3);
                }
                Form_Grid.RowStyles[i + 1].SizeType = SizeType.AutoSize;
                i++;
            }

            this.Size = new Size(280, Height);
            MySql.Close();
        }
        catch (Exception ex) { Notify.Errors(ex.HResult, ex.ToString(), ex.Message); }
    }

VB.Net Code

Public Labels, Entries As New List(Of Object)
Public Form_Grid As New TableLayoutPanel

Private Sub Worklog_GetFormat()
    Try
        Dim MySql As MySqlConnection = New MySqlConnection("Server=10.5.1.97;Database=" & Session.Department & ";Uid=" & Session.Username & ";Pwd=PocketUser2015;")
        MySql.Open()
        Dim MySqlCmd As MySqlCommand = New MySqlCommand("SELECT * FROM wl_format", MySql)
        Dim Reader As MySqlDataReader = MySqlCmd.ExecuteReader

        Dim i As Integer = 0
        Dim LogHeight As Integer = 255
        Do While Reader.Read
            Dim Name As String = Reader.GetString("Name")
            Dim Type As String = Reader.GetString("Type")

            Dim L As New Label
            With L
                If CBool(Reader.GetInt32("Required")) Then .Text = Name & "*" Else .Text = Name
                .Name = "L" & i
                .AutoSize = True
                .TextAlign = ContentAlignment.MiddleLeft
                .Visible = True
                .Font = New Font("Microsoft Sans Serif", 8.25)
                .Dock = DockStyle.Fill
                .Margin = New Padding(0, 3, 0, 3)
                If Not Labels.Contains(L) Then
                    Labels.Add(L)
                    AddHandler .MouseUp, AddressOf Labels_MouseUp
                End If
            End With
            Form_Grid.Controls.Add(L, 0, i + 1)
            Form_Grid.RowStyles(i + 1).SizeType = SizeType.AutoSize

            Select Case True
                Case Type = "Textbox" Or Type = "Suggestbox"
                    Dim T As New TextBox
                    LogHeight += 26
                    With T
                        .Name = "T" & i
                        .Visible = True
                        .Dock = DockStyle.Fill
                        .TabIndex = i + 1
                        If Type = "Suggestbox" Then
                            .AutoCompleteMode = AutoCompleteMode.SuggestAppend
                            .AutoCompleteSource = AutoCompleteSource.CustomSource
                            Dim p = Split(Options(i), "|")
                            For o = 0 To p.Count - 1
                                T.AutoCompleteCustomSource.Add(p(o))
                            Next
                        End If
                        If Not Entries.Contains(T) Then
                            Entries.Add(T)
                            AddHandler .KeyPress, AddressOf Entries_KeyPress
                        End If
                    End With
                    Form_Grid.Controls.Add(T, 1, i + 1)
                    Form_Grid.SetColumnSpan(T, 3)
                Case Type = "Dropbox"
                    Dim D As New ComboBox
                    LogHeight += 26
                    With D
                        .Name = "D" & i
                        .Visible = True
                        .Dock = DockStyle.Fill
                        .TabIndex = i + 1
                        .AutoCompleteMode = AutoCompleteMode.Suggest
                        .AutoCompleteSource = AutoCompleteSource.ListItems
                        .FormattingEnabled = True
                        .Sorted = True
                        Dim p = Split(Options(i), "|")
                        For o = 0 To p.Count - 1
                            D.Items.Add(p(o))
                        Next
                        If Not Entries.Contains(D) Then
                            Entries.Add(D)
                            AddHandler .SelectedValueChanged, AddressOf Dropbox_ValueChanged
                        End If
                    End With
                    Form_Grid.Controls.Add(D, 1, i + 1)
                    Form_Grid.SetColumnSpan(D, 3)
                Case Type = "Numberbox"
                    Dim N As New NumericUpDown
                    LogHeight += 30
                    With N
                        .Name = "N" & i
                        .Visible = True
                        .Dock = DockStyle.Fill
                        .TabIndex = i + 1
                        .BorderStyle = BorderStyle.FixedSingle
                        If Not Entries.Contains(N) Then
                            Entries.Add(N)
                        End If
                    End With
                    Form_Grid.Controls.Add(N, 1, i + 1)
                    Form_Grid.SetColumnSpan(N, 3)
                Case Type = "Checkbox"
                    Dim C As New CheckBox
                    LogHeight += 35
                    With C
                        .Name = "C" & i
                        .Visible = True
                        .Dock = DockStyle.Fill
                        .TabIndex = i + 1
                        .Text = L.Text
                        L.Visible = False
                        If Not Entries.Contains(C) Then
                            Entries.Add(C)
                        End If
                    End With
                    Form_Grid.Controls.Add(C, 1, i + 1)
                    Form_Grid.SetColumnSpan(C, 3)
            End Select
            Form_Grid.RowStyles(i + 1).SizeType = SizeType.AutoSize

            i += 1
        Loop

        Me.Size = New System.Drawing.Size(280, LogHeight)
        MySql.Close()
    Catch ex As Exception
        Notify.Errors(ex.HResult, ex.ToString, ex.Message)
    End Try
End Sub
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

Your code should be split for improved readability and easier maintenance (code below is not tested)

1) Connection and command construction can be separated. Usually data access operations are handled in a separated layer (assembly) - do not mix Data Access Logic (DAL) with UI. This is particularly important when application is migrated to 3-tier arhitecture (UI client talks to a application server, Web Server etc. rather than talking directly to the database).

Also, connection, command and reader should be disposed

private String _connectionStringFormat = "Server=localhost;Database={0};Uid={1};Pwd=Password2016;";

private MySqlConnection GetConnection(String department, String username)
{
   var connectionStr = String.Format(_connectionStringFormat, department, username);
   var connection = new MySqlConnection(connectionStr);
   connection.Open();
}

private void executeReader(MySqlConnection connection, String command, Action<MySqlDataReader> readAction)
{
   using (MySqlCmd = new MySqlCommand(command, connection))
   {
       using (var reader = MySqlCmd.ExecuteReader())
       {
           readAction(reader);
       }
   }

}

public static List<object> _labels = new List<object>();
public static List<object> _entries = new List<object>();
public static TableLayoutPanel Form_Grid = new TableLayoutPanel();

private void wl_getFormat()
{
    MySqlConnection connection = null;
    int i = 0; 
    int Height = 255;

    try
    {
       connection = GetConnection(Session.Department, Session.Username);

       // consider replacing * with the actual columns selected
       executeReader(connection, "SELECT * FROM wl_format", reader => 
          {
               // your code inside while (Reader.Read()) comes here
          };

        this.Size = new Size(280, Height);
     }
     catch (Exception ex) 
     { 
        Notify.Errors(ex.HResult, ex.ToString(), ex.Message); 
     }
     finally
     {
         // this executes regardless of exception existence
         // connection should be closed and disposed
         if (connection.State == System.Data.ConnectionState.Open)
         {
            connection.Close();
            connection.Dispose();
         }
     }
}

2) * usage in queries (see comment above) - it is better to avoid it, as wl_format table might grow in time and extra columns might not be needed in some contexts

3) finally - it is best to use finally when some actions must take place regardless of how the code executed (with exception or not). Closing the connection is one of these things (see above)

4) Constants instead of pure strings - it is better to use constants to define the strings that sound generic like "TextBox", "Suggestbox" etc. (maybe this is not the only place that's defining the interface dynamically):

public const String ControlTypeTextBox = "TextBox";
public const String ControlTypeDropbox = "Dropbox";

5) Try to minimize cyclomatic complexity (less if ... else ...)

private void finalizeRow(ref int i)
{
   Form_Grid.RowStyles[i + 1].SizeType = SizeType.AutoSize;
   i++;
}

string Name = Reader.GetString("Name"); //Get field name
string Type = Reader.GetString("Type"); //Get field type

// Required label here

if (Type == "Textbox")
{
    TextBox T = new TextBox();
    Height += 26;

    T.Name = "T" + i;
    T.Dock = DockStyle.Fill;
    T.TabIndex = i + 1;

    // ...
    finalizeRow(ref i);
    continue;
}

if (Type == "Suggestbox")
{
    TextBox T = new TextBox();
    Height += 26;

    T.Name = "T" + i;
    T.Dock = DockStyle.Fill;
    T.TabIndex = i + 1;
    if (Type == "Suggestbox")
    {
        T.AutoCompleteMode = AutoCompleteMode.SuggestAppend;
        T.AutoCompleteSource = AutoCompleteSource.CustomSource;
        string[] p = _options[i].Split('|');
        foreach (string a in p)
        {
            T.AutoCompleteCustomSource.Add(a);
        }
    }

    // ...
    finalizeRow(ref i);
    continue;
}

// and so on...

6) factories for controls

Each control can be constructed in a separate function. E.g:

private Label getRequiredLabel(int i, bool isRequired) 
{ 
    Label L = new Label();

    if (isRequired)
        L.Text = Name + "*";
    else 
        L.Text = Name; 

    L.Name = "L" + i;
    L.AutoSize = true;
    L.TextAlign = ContentAlignment.MiddleLeft;
    L.Visible = true;
    L.Font = new Font("Microsoft Sans Serif", 8);
    L.Dock = DockStyle.Fill;
    L.Margin = new Padding(0, 3, 0, 3);

    if (!_labels.Contains(L))
    {
        _labels.Add(L);
        L.MouseUp += new MouseEventHandler(wl_Lock);
    }

    Form_Grid.Controls.Add(L, 0, i + 1);
    Form_Grid.RowStyles[i + 1].SizeType = SizeType.AutoSize;
}

and use it like:

var L = getRequiredLabel(i, Reader.GetInt32("Required") == 1);

These tricks help you start getting along with separating things. Robert Snyder's suggestions are also very good.

Reading your question also made me curious about Entity Framework (EF) and MySql. It looks like they can be used together.

\$\endgroup\$
1
  • \$\begingroup\$ I love ideas like these, simple things I probably wouldn't have thought of. \$\endgroup\$
    – Aux
    Jan 12, 2016 at 15:40
3
\$\begingroup\$

Separate Concerns

The very first few lines in your code for wl_getFormat() startup and use a MySqlConnection. While that in of itself is not wrong, how you use them is not right. Ideally you should at the bare minimum pull out the MySqlConnection stuff into a new class that returns a IEnumerable<FormFields>. FormFields being a new class that has 3 properites (guessing by the code you provide). We do this for a variety of reasons. One is that we want a class to do a single thing. In this case we want this class to give us all the Fields on this form with a small amount of information about each. There are many approaches on how to do this, and how to make it testable so I won't go into too much detail about it. But just know that just doing that makes understanding your code MUUUUUCH easier. For instance now my code would read something like this

        var fields = new FormField[0];
        try
        {
            fields = new FormFieldRepository().GetAllFields();
        }
        catch (Exception ex)
        {
            Notify.Errors(ex.HResult, ex.ToString(), ex.Message);
        }
        foreach (var field in fields)
        {
            //... build your code from here. without the try/catch
        }

that probably is not the exact code I would end up at, but it is the first step. I use the Repository code to connect to something (in your case MySql) parse through all the information and give me the important parts. If there is an error somehow while getting those fields I can easily just return or show a notification or something. All of that bit of code is easily seen as the try/catch is around very little code. (another tip).

since I instantiated the fields to be an empty array if something did go wrong the foreach wouldn't cause any sort of problems for me as it wouldn't execute any code inside of it.

Small Try Catches

I like the phrase I heard on here the other day about Pokemon catching. We don't want to just blanket all our code with a generalized Try/Catch. It tell us nothing, and just adds a larger level of nesting. Typically you only want a try/catch around the small bits of code that you expect to throw an exception now and then. This is the reason why I have fields instantiated outside of the fetching of all the fields, and program against knowing that potentially there are no fields.

Ending notes

there are a few other things, but I have to get going, so I will end on this note. If you use C# or Vb.Net design patterns in your code will help you encapsulate things in such a way that is easy to understand both now, and later when you have to change the code. Look into a few (such as the flyweight, or the builder pattern) as they could help encapsulate a different portions of your code into much more manageable sizes.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.