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:

enter image description here

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);
 }
}

• BCdotWEB: The current question title applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?.
• BCdotWEB: Why don't you use Dapper?
• BCdotWEB: Don't mix UI logic and DB queries. Abstract to something like MVVM. See softwareengineering.stackexchange.com/questions/277143/… .
• DeveloperLV: @BCdotWEB I have looked into using MVVM but it does not make sense for me. I don't even know how to apply it to my current project.
• Kate: I suppose you have a form where the controls (radio buttons) are already laid out statically, but instead you could add them dynamically to the form as you are reading from the database. Do you already have a table that contains possible categories ? Then it should be easy to fix the problem and you'll have dynamic layout, hence freedom to add more categories in the future without having to redesign your form.

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.

• DeveloperLV: Really appreciate your answer! - Will look into it when I am back at work and try to implement everything you suggested :D

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.

• DeveloperLV: Hi - Thanks for your answer. I completely agree with re-thinking the design of the form. It's only a start at the moment.. but eventually I'll get a decent form created :D I will keep your comments in mind and think of it when I back at it again.