Getting data from database and present it on Winforms

Question

Currently, I am working on this project where I need to achieve the following...

Get column named Category from MySQL database. And whatever the output is, check the specific radio button on winforms.

I'd like to see if I can improve it, or if there is a much easier way of achieving my goal:

Personally - i think there is too many if, else if statements...

Radio Buttons:

Code:

void function()
{
    try
    {
        using(var conn = new MySqlConnection(ConnectionString.ConnString))
        {
            string query = "select category from customer_complaints_actions where id = @id";

            conn.Open();
            using(var cmd = new MySqlCommand(query, conn))
            {
                cmd.Parameters.AddWithValue("@id", Convert.ToInt32(complaints_data.SelectedRows[0].Cells[0].Value.ToString()));
                using(MySqlDataReader read = cmd.ExecuteReader())
                {
                    while (read.Read())
                    {

                        // Categories
                        if ((read["category"].ToString()) == "Retained Pitt/Shell")
                        {
                            Pitt_Shell.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Mineral Stone")
                        {
                            mineral_stone.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Extraneous Vegetable Matter")
                        {
                            vegetable_matter.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Paper/Cardboard")
                        {
                            paper_cardboard.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Wood")
                        {
                            wood.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "String")
                        {
                            _string.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Hair")
                        {
                            hair.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Rubber")
                        {
                            rubber.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Metal")
                        {
                            metal.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Other Foreign Body")
                        {
                            other_foreign_body.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Taste")
                        {
                            taste.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Texture")
                        {
                            texture.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Smell / Odour")
                        {
                            smell.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Appearance")
                        {
                            appearance.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Off / Mould")
                        {
                            off_mould.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Crystallised / Bloomed")
                        {
                            bloomed.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Insect Damage / Infestation")
                        {
                            infestation.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Other Organoleptic")
                        {
                            other_organoleptic.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Light Weight Pack")
                        {
                            light_weight_pack.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Product Caught in Seals")
                        {
                            product_caught_in_seal.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Shepcote Damage")
                        {
                            shepcote_damage.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Courier Damage")
                        {
                            courier_damage.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Incorrect Packaging / Label")
                        {
                            incorrect_pack_label.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Other Packing / Packaging")
                        {
                            other_pack.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Short Shelf Life")
                        {
                            short_shelf_life.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Out of Date")
                        {
                            out_of_date.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Picking Error")
                        {
                            picking_error.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Order Input Error")
                        {
                            order_input_error.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Customer Order Error / Change")
                        {
                            customer_order_error.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Delivery Issues (Wrong Location etc.)")
                        {
                            delivery_issues.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Other Order / Delivery")
                        {
                            other_delivery.Checked = true;
                        }
                        else if ((read["category"].ToString()) == "Injury")
                        {
                            injury.Checked = true;
                        }

                    }
                    read.Close();
                    conn.Close();
                }
            }
        }
    }
    catch (Exception e)
    {
        MessageBox.Show(e.ToString(), "Error", MessageBoxButtons.OK, MessageBoxIcon.Warning);
    }
}

Answer 1

Notes :

• Don't mix UI logic with Business logic, separate them using classes, and you can use one of the known design patters such as MVVM. (as @BCodotWEB noted).

• When you use using, there is no need to close the connection or dispose the object such as calling Dispose() or Close() within the using block. As the using block will do that for you whenever it reaches the end of the block. So, in your code, conn and cmd will be disposed automatically.

• When you compare strings, ensure that you specify the StringComparison.OrdinalIgnoreCase to avoid Case Sensitively comparison.

• When you have multiple if/else statments on just one value (like yours), convert it to switch statement, for better readability and maintainability.

• don't use MySqlCommand nor any DbCommand derived class directly, as it's barebone classes, meaning, it will require you to handle everything from the connection, commands, results, types conversations, modeling ..etc. Which is too much work.(Not to mention SQL Injection). Instead, use managed Object–relational Mapping (ORM) framework, such as Dapper and Entity Framework that are built around ADO.NET which saves time, and effort, and also has a better implementation and handling.

Coming back to your main concern :

I'd like to see if I can improve it, or if there is a much easier way of achieving my goal:

Personally - i think there is too many if, else if statements...

it seems that your database has category table, which has predefined values, and the customer_complaints_actions table saves category values and not the foreign key of the category table. This would be a design issue. Even if the values are matching, there is no guarantee of keeping them intact. For instance, if for some reason someone decided to add just an s to the Wood category name to be plural, then it will invalidate all related records that uses Wood category, and you will need to update them with this change as well. But, if you used the FK, then any changes to the category name, it won't affect the results. Because it's linked with the Key and not the Value. If you change the Key then it will do a cascade update on all its FKs.

To solve that, you'll need to first to fix the database structure design and follow database principles and best practices. Then, in your code, you will need to use ORM and also applying at least one of the known design patterns that would fit your application requirements.

If you do this, then, you can make your application more dynamic and fixable for any business changes.

For instance, instead of creating RadioButton for each category manually, you could make your Form dynamically on the form initiation, based on the category table values.

So, first thing you need to get all Category table values when the Form is initiated, something like this (Using Entity Framework as an example) :

public FormCustomerComplaint() {
    
    InitiateCategories();
}

private void InitiateCategories()
{
    // Looping over Category Table Rows 
    // context represents the database in Entity Framework
    foreach(var category in context.Categories)
    {
        var radio = new RadioButton
        {
            Name = $"CatId{category.Id}",
            Text = category.Name, 
        };

        GroupBoxCategory.Controls.Add(radio);
    }
}

Now, even if there is a new change on the Category table, it'll be affected automatically.

Then, you can use the same concept to reterive the values for each record in the customer_complaints_actions table, something like this (again, using Entity Framework as an example) :

// Now to get the category from customer_complaints_actions
// Assuming that `categoryId` is the FK for `Category` table. 
// context represents the database in Entity Framework
var result = context.CustomerComplaintsActions.FirstOrDefault(x=> x.Id == id);

if(result != null)
{   
    
    var control = GroupBoxCategory.Controls.Find($"CatId{result.CategoryId}", true).FirstOrDefault();
    
    if(control != null) 
    {
        var radio = (RadioButton) control; 
        radio.Checked = true;                       
    }
}

These are just simple examples to show you how easy would be when you using the proper techniques to work with your application. I used them to encourage you to start using ORM (not focusing on the design patterns here). Just start with that, andif you found yourself good at using it, then you can start using a design pattern that would fit your application needs such as object-modeling and MVVM to make your code easy to manage.

Answer 2

You already got a good review from iSR5. So perhaps I am not going to comment a lot on the code. My take is on ergonomics. Like all (?) developers I am lazy but I like challenges, however I am willing to think outside the box and devise a better solution before tackling a new problem.

After a more careful look I am thinking that you might want to reconsider form design. A combo box bound to a datatable may be a better option if the number of items becomes consequential, and would optimize form space. A combo box can also provide autocomplete facilities, and thus speed up encoding. Since you are using radio buttons, it means only one answer is possible per category, then a combo box is a valid alternative.

As it is the form already looks pretty crowded, that means lots of back and forth mouse movements to get from one control to another, considering the width of the form. I even predict elbow pain under heavy use conditions. If you have to add more options in the future, that could be problematic from a usability point of view. The form will become overwhelming and unengaging.

Moreover, the items do not seem to be sorted. Unless the user is very accustomed to the form, he/she has to visually scan the form every time to locate the desired option. Hint: a column layout could make a difference and consider sorting items (unless they are actually sorted in terms of frequency based on your business experience). Either way, the options should be arranged in a way that is intuitive and not random.

Basically, you could have a more compact form with 5 combo boxes instead of the current 5 radio button zones you have now, and each combo box can be fed by a table. Thus you can have a form with a static layout but dynamic values, and its size will always remain contained.

Another possible trick that I have used sometimes is to place a group of dynamic controls inside a flowlayoutpanel in a kind of column layout, such a panel can have a vertical scrollbar and even autosize. Here is an example from Stackoverflow with picture. This could be an alternative as long as you don't have to too many items to scroll.