C# Window Form Application Loading data into DataGrid and highlight specific values based on their values

Question

I'd like to hear some reviews or tips where I can improve in my code - or better if you can post your own version of the code from somebody experienced where I can visually see to why you have coded it in your way rather than my way.

What this code does is simply loads data into the dataGridView using Windows Forms and I have multiple functions such as the search bar.

Function such as Add new document, Update selected document and View File code will not be seen as it is on a separate Form. But will ask a separate question for each when I have some reviews on this code.

How the app looks

FSQMMain.cs

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using MySql.Data.MySqlClient;
using MySqlX.XDevAPI.Relational;
using Technical_Application.Classes;
using System.Diagnostics;

namespace Technical_Application.Forms.SubForm.FSQM
{
    public partial class FSQMMain : Form
    {
        public FSQMMain()
        {
            InitializeComponent();
        }

        private void ChooseSection_Load(object sender, EventArgs e)
        {
            //Load data with MySQL values
            LoadData();

            //
            //Only show buttons depending on the user access id, taken from MySQL column `access id`
            if (UserDetails.accessId == 2 || UserDetails.accessId == 3)
            {
                addNewDocument.Visible = false;
                bunifuFlatButton1.Visible = false;
            }
            //
            //

        }


        //This is a function to load data to the Data Grid View
        public void LoadData()
        {
            try
            {
                //Load data from MySQL table
                //Connection string 
                using (var conn = new MySqlConnection(ConnectionString.ConnString))
                {
                    //Creating a data adapter and filling the table with dataSet
                    using (var mySqlDataAdapter = new MySqlDataAdapter("SELECT f.id, " +
                                                                       "f.Document_Reference," +
                                                                       " f.document_name, " +
                                                                       "f.path, " +
                                                                       "f.version, " +
                                                                       "f.section, " +
                                                                       "date(f.last_review_date), " +
                                                                       "date(f.review_date)," +
                                                                       " u.username, " +
                                                                       "f.date_modified " +
                                                                       "from files f join users u on f.user_modified = u.id;", conn))
                    {
                        using (var dataSet = new DataSet())
                        {
                            DataSet DS = new DataSet();
                            mySqlDataAdapter.Fill(DS);
                            data.DataSource = DS.Tables[0];
                            //Assign Header Titile for each column loaded
                            data.Columns[0].HeaderText = "ID";
                            data.Columns[1].HeaderText = "Document Reference";
                            data.Columns[2].HeaderText = "Document Name";
                            data.Columns[3].HeaderText = "Path";
                            data.Columns[4].HeaderText = "Version Number";
                            data.Columns[5].HeaderText = "Section";
                            data.Columns[6].HeaderText = "Last Review Date";
                            data.Columns[7].HeaderText = "Next Review Date";
                            data.Columns[8].HeaderText = "User Last Modified";
                            data.Columns[9].HeaderText = "Modified Date";

                            //Hide Specific columns
                            data.Columns[0].Visible = false;
                            data.Columns[3].Visible = false;
                        }
                    }
                }
            }

            //If any errors - then stop Execution
            catch (Exception ex)
            {
                MessageBox.Show(ex.ToString(), "Error", MessageBoxButtons.OK, MessageBoxIcon.Warning);
                //Stop executing code
                return;
            }
        }

        //Search button
        private void searchbtn_Click(object sender, EventArgs e)
        {
            //We use a string builder, becasue we have 3 different search boxes to use.
            StringBuilder sb = new StringBuilder();

            //Here we are seeing if the textboxes have any values
            if (documentName.Text.Length > 0)
            {
                //If they do - add a search filter
                sb.Append($"document_name like '%{documentName.Text}%'");
            }

            if (docRef.Text.Length > 0)
            {
                if (sb.Length > 0)
                {
                    sb.Append(" and ");
                }

                
                sb.Append($"document_reference like '%{docRef.Text}%'");
            }

            if (section.Text.Length > 0)
            {
                if (sb.Length > 0)
                {
                    sb.Append(" and ");
                }

                sb.Append($"section ={Int32.Parse(section.Text)}");
            }

            //Combine all filters together
            (data.DataSource as DataTable).DefaultView.RowFilter = sb.ToString();

        }

        //Only allow numbers in `Section` text box
        private void section_TextChanged(object sender, EventArgs e)
        {
            if (System.Text.RegularExpressions.Regex.IsMatch(section.Text, "[^0-9]"))
            {
                MessageBox.Show("Please enter only numbers.", "Integer - Error", MessageBoxButtons.OK, MessageBoxIcon.Warning);
                section.Text = section.Text.Remove(section.Text.Length - 1);
            }
        }

        //Clear search boxes
        private void bunifuFlatButton2_Click(object sender, EventArgs e)
        {
            LoadData();
            documentName.Clear();
            section.Clear();
            docRef.Clear();
            docRef.Select();
        }


        //Open form add new document
        private void addNewDocument_Click(object sender, EventArgs e)
        {
            //Open NewDocument Form
            bool Isopen = false;

            foreach (Form f in Application.OpenForms)
            {
                if (f.Text == "New Document")
                {
                    Isopen = true;
                    f.BringToFront();
                    if (f.WindowState == FormWindowState.Minimized)
                    {
                        f.WindowState = FormWindowState.Normal;
                    }
                    break;
                }
            }
            if (Isopen == false)
            {
                var newDoc = new AddNewDocument(this);
                newDoc.Show();
            }

        }


        //Format cells to red, amber and green depending on the review date
        private void data_CellFormatting(object sender, DataGridViewCellFormattingEventArgs e)
        {
            //Set variable today to todays date
            DateTime today = DateTime.Today.Date;

            //Iterate through each row and check the review date
            foreach (DataGridViewRow Myrow in data.Rows)
            {
                DateTime dt = DateTime.Parse(Myrow.Cells[7].Value.ToString());
                //If today or past review date, show as red
                if (dt <= today)
                {
                    Myrow.DefaultCellStyle.BackColor = Color.Red;
                }
                //If review date is within 30 days
                else if ((dt - today).TotalDays <= 30)
                {
                    Myrow.DefaultCellStyle.BackColor = Color.FromArgb(231, 202, 0);

                }
                //Else show green
                else
                {
                    Myrow.DefaultCellStyle.BackColor = Color.FromArgb(20, 205, 14);
                }
            }
        }

        //Update button
        private void bunifuFlatButton1_Click(object sender, EventArgs e)
        {
            bool Isopen = false;

            foreach (Form f in Application.OpenForms)
            {
                if (f.Text == "Update")
                {
                    Isopen = true;
                    f.BringToFront();
                    if (f.WindowState == FormWindowState.Minimized)
                    {
                        f.WindowState = FormWindowState.Normal;
                    }
                    break;
                }
            }
            if (Isopen == false)
            {
                if (this.data.SelectedRows.Count > 0)
                {
                    var update = new Update(this);
                    update.Show();
                }
                else
                {
                    MessageBox.Show("No Records Selected", "", MessageBoxButtons.OK, MessageBoxIcon.Warning);
                }
            }
        }

        //View file Button
        private void viewFile_Click(object sender, EventArgs e)
        {
            if (this.data.SelectedRows.Count > 0)
            {
                
                bool Isopen = false;

                foreach (Form f in Application.OpenForms)
                {
                    if (f.Text == "File Viewer")
                    {
                        Isopen = true;
                        f.BringToFront();
                        if (f.WindowState == FormWindowState.Minimized)
                        {
                            f.WindowState = FormWindowState.Normal;
                        }
                        break;
                    }
                }
                if (Isopen == false)
                {
                    if (this.data.SelectedRows.Count > 0)
                    {
                        var fileView = new File_Viewer(this);
                        fileView.Show();
                    }
                    else
                    {
                        MessageBox.Show("No Records Selected", "", MessageBoxButtons.OK, MessageBoxIcon.Warning);
                    }
                }
            }
            else
            {
                MessageBox.Show("No Records Selected", "", MessageBoxButtons.OK, MessageBoxIcon.Warning);
            }
        }


    }
}

Answer 1

Don't use StringBuilder in searchbtn_Click; use a Joined List<string>

You're combining a small number of small strings, so there's likely no real performance benefits in using a StringBuilder.

I would suggest creating a List<string> and then calling string.Join on the list:

var parts = new List<string>();
if (documentName.Text.Length > 0) {
    parts.Add($"document_name like '%{documentName.Text}%'");
}
if (docRef.Text.Length > 0) {
    parts.Add($"document_reference like '%{docRef.Text}%'");
}
if (section.Text.Length > 0) {
    parts.Add($"section ={Int32.Parse(section.Text)}");
}
(data.DataSource as DataTable).DefaultView.RowFilter = string.Join(" and ", parts);

I might go even further and build the controls and formatted parts into some kind of collection of pairs, which could then be filtered and the appropriate results passed into Join:

var parts = new (string text, string part)[] {
        (documentName.Text, $"document_name like '%{documentName.Text}%'"),
        (docRef.Text, $"document_reference like '%{docRef.Text}%'"),
        (section.Text, $"section = {Int32.Parse(section.Text)}")
    }
    .Where(x => x.text.Length > 0)
    .Select(x => x.part);
(data.DataSource as DataTable).DefaultView.RowFilter = string.Join(" and ", parts);

---

Refactor form-finding code into a separate method

You're searching in multiple places for a form based on the value of the form's Text property. I would suggest you refactor this code into a separate method; something like this:

private Form findForm(string text) {
    var found = Application.OpenForms.Cast<Form>().FirstOrDefault(f => f.Text == text);

    // C# 8 syntax; for earlier versions use 'if (found != null) {
    if (found is {}) {
        found.BringToFront();
        if (found.WindowState == FormWindowState.Minimized) {
            found.WindowState = FormWindowState.Normal;
        }
    }
    return found;
}

Then, for example in addNewDocument_Click, you could write:

var found = findForm("New Document");
if (found is null) {
    var newDoc = new AddNewDocument(this);
    newDoc.Show();
}

---

Consider putting the SQL statement in a separate const. This makes it easier to see what's going on at the start of your using block.

Since you're not using the inner dataSet variable, you can remove the innermost using.

I would also suggest making use of the new using statement in C# 8 if you can:

using var conn = new MySqlConnection(ConnectionString.ConnString);
using var mySqlDataAdapter = new MySqlDataAdapter(sql, conn);
DataSet DS = new DataSet();
mySqlDataAdapter.Fill(DS);
...

Alternatively, use the single-statement using form for the outer using:

using (var conn = new MySqlConnection(ConnectionString.ConnString))
using (var mySqlDataAdapter = new MySqlDataAdapter(sql, conn)) {
    DataSet DS = new DataSet();
    mySqlDataAdapter.Fill(DS);
    ...
}

Answer 2

This:

if (UserDetails.accessId == 2 || UserDetails.accessId == 3)

Consider using an Enum instead of hardcoding integer values eg:

enum AccessLevel : int
{
    None = 0,
    Manager = 2,
    SuperUser = 3
}

---

You can use verbatim strings for your SQL statements:

string sql = @"SELECT f.id, f.Document_Reference, f.document_name,
f.path, f.version, f.section,
date(f.last_review_date), date(f.review_date),
u.username, f.date_modified
from files f join users u on f.user_modified = u.id;";

---

There is repetitive code: bunifuFlatButton1_Click and viewFile_Click are almost the same. Surely you can merge both into one function and pass the control name as argument. Then your code becomes and shorter and easier to maintain.

---

Here you set the header titles for your DGV:

                        //Assign Header Titile for each column loaded
                        data.Columns[0].HeaderText = "ID";
                        data.Columns[1].HeaderText = "Document Reference";
                        data.Columns[2].HeaderText = "Document Name";
                        data.Columns[3].HeaderText = "Path";
                        data.Columns[4].HeaderText = "Version Number";
                        data.Columns[5].HeaderText = "Section";
                        data.Columns[6].HeaderText = "Last Review Date";
                        data.Columns[7].HeaderText = "Next Review Date";
                        data.Columns[8].HeaderText = "User Last Modified";
                        data.Columns[9].HeaderText = "Modified Date";

                        //Hide Specific columns
                        data.Columns[0].Visible = false;
                        data.Columns[3].Visible = false;

There is nothing wrong with that but personally I prefer to separate design from programming logic as much as possible. So I would just edit the DGV in design mode and put the header titles directly in it. You can also hide some columns by default. That makes the code shorter (less scrolling). The one thing that I change at runtime is the column width so that it fits the cell contents. Most attributes are otherwise static.

---

I think you can can simplify this part:

    //Open NewDocument Form
    bool Isopen = false;

    foreach (Form f in Application.OpenForms)
    {
        if (f.Text == "New Document")
        {
            Isopen = true;
            f.BringToFront();
            if (f.WindowState == FormWindowState.Minimized)
            {
                f.WindowState = FormWindowState.Normal;
            }
            break;
        }
    }
    
    if (Isopen == false)
    {
        var newDoc = new AddNewDocument(this);
        newDoc.Show();
    }

Basically you can check if a form is open or not in a more concise fashion:

public bool CheckIfFormIsOpen(string formname)
{
    bool formOpen= Application.OpenForms.Cast<Form>().Any(form => form.Name == formname);

    return formOpen;
}

Have a look here

---

The good thing is that there are adequate comments and the code is quite clear to understand.

---

You can probably declutter the code further by removing unused imports unless you have more code in this form that relies on those imports.