2
\$\begingroup\$

I have a DataGridView which displays an employee list. I use the employee ID to filter my global employee list and display information about the selected employee in various listboxes. This is my first coding project, and I'm pleased that it's working well, but I do feel like I'm repeating myself a lot. Is this an instance where an experienced coder would be able to abstract some underlying pattern and write a method which simplifies everything?

Also, after setting the datasources, I have a comment, "CREATE DATASOURCE LIST". I've heard that comments shouldn't say WHAT code does, but rather WHY it does something. But I find the green comments in caps makes it really easy to find sections of my code at a glance. I also have comments to separate the various Tabs on my Winform application. "INVENTORY TAB", "EMPLOYEE TAB", "VEHICLES TAB", etc. Are those frowned upon?

  private void employeeGridView_CellClick(object sender, DataGridViewCellEventArgs e)
            {
                DataGridViewCell selectedEmployeeCell = employeeGridView.CurrentCell;
                int selectedEmployeeRow = selectedEmployeeCell.RowIndex;
                int selectedEmployeeID = (int)employeeGridView.Rows[selectedEmployeeRow].Cells[0].Value;

                certificationsListBox.DataSource = certificationListBoxString(id: selectedEmployeeID, employees: globalEmployeeList.ToList());
                citationsListBox.DataSource = citationsListBoxString(id: selectedEmployeeID, employees: globalEmployeeList.ToList());

                emailGridView.DataSource = null;
                emailGridView.DataSource = GetEmailRecordsList(SelectedEmployeeID: selectedEmployeeID);
                emailGridView.Columns[0].Visible = false;
                emailGridView.RowHeadersVisible = false;
                emailGridView.Columns[1].Width = 200;
                emailGridView.Columns[2].Width = 75;

                phoneGridView.DataSource = null;
                phoneGridView.DataSource = GetPhoneRecordsList(SelectedEmployeeID: selectedEmployeeID);
                phoneGridView.Columns[0].Visible = false;
                phoneGridView.RowHeadersVisible = false;
                phoneGridView.Columns[1].Width = 150;
                phoneGridView.Columns[2].Width = 75;

                DepartmentListBox.DataSource = null;
                DepartmentListBox.DataSource = GetDepartment(SelectedEmployeeID: selectedEmployeeID);
                DepartmentListBox.ClearSelected();

                StatusListBox.DataSource = null;
                StatusListBox.DataSource = GetStatus(SelectedEmployeeID: selectedEmployeeID);
                StatusListBox.ClearSelected();

                HireDateListBox.DataSource = null;
                HireDateListBox.DataSource = GetHireDate(SelectedEmployeeID: selectedEmployeeID);
                HireDateListBox.ClearSelected();
            }

        //CREATE DATASOURCE LISTS
        public static IList<EmployeeModel> globalEmployeeList = new List<EmployeeModel>();
        public static async Task<IList<EmployeeModel>> InitializeEmployeeList()
        {
            globalEmployeeList = await GlobalConfig.Connection.GetEmployeeList();

            foreach (EmployeeModel eModel in globalEmployeeList)
            {
                var groupedEmailList = new List<EmailModel>();
                var groupedPhoneList = new List<PhoneModel>();
                List<int> phoneIDs = new List<int>();
                List<int> emailIDs = new List<int>();
                foreach (EmailModel emailModel in eModel.EmailList)
                {
                    if (!emailIDs.Contains(emailModel.ID))
                    {
                        emailIDs.Add(emailModel.ID);
                        groupedEmailList.Add(emailModel);
                    }
                }
                eModel.EmailList = groupedEmailList;

                foreach (PhoneModel phoneModel in eModel.PhoneList)
                {
                    if (!phoneIDs.Contains(phoneModel.ID))
                    {
                        phoneIDs.Add(phoneModel.ID);
                        groupedPhoneList.Add(phoneModel);
                    }
                }
                eModel.EmailList = groupedEmailList;
                eModel.PhoneList = groupedPhoneList;
            }
            return globalEmployeeList;
        }
               
        private IList<PhoneModel> phoneRecords = new List<PhoneModel>();
       
        private IList<PhoneModel> GetPhoneRecordsList(int SelectedEmployeeID)
        {
            if (phoneRecords != null)
            {
                phoneRecords.Clear();
            }
            foreach (EmployeeModel em in globalEmployeeList.Where(person => person.ID == SelectedEmployeeID))
            {
                foreach (PhoneModel pm in em.PhoneList)
                {
                    phoneRecords.Add(pm);
                }
            }
            return (IList<PhoneModel>)phoneRecords;
        }

        public IList<EmailModel> emailRecords = new List<EmailModel>();
        
        public IList<EmailModel> GetEmailRecordsList(int SelectedEmployeeID)
        {
            if (emailRecords != null)
            {
                emailRecords.Clear();
            }
            foreach (EmployeeModel em in globalEmployeeList.Where(person => person.ID == SelectedEmployeeID))
            {
                foreach (EmailModel emm in em.EmailList)
                {
                    emailRecords.Add(emm);
                }
            }
            return (IList<EmailModel>)emailRecords;
        }

    /* TURN THIS INTO A WHILE LOOP ONCE YOU LEARN HOW TO DO WHILE LOOPS  
    ALSO, THERE IS NO REASON FOR GETSTATUS() OR GETDEPARTMENT() TO RETURN LISTS 
    OF STRINGS.  I WAS CLEARLY JUST REPEATING THE CODE FROM EMAIL AND PHONE LISTS 
    BECAUSE IT WORKED, BUT I SHOULD REFACTOR SO THE FUNCTIONS RETURN SIMPLE 
    STRINGS. */    

public IList<string> selectedStatus = new List<string>();
        public IList<string> GetStatus(int SelectedEmployeeID)
        {
            if (selectedStatus != null)
            {
                selectedStatus.Clear();
                foreach (EmployeeModel em in globalEmployeeList.Where(person => person.ID == SelectedEmployeeID))
                {
                    selectedStatus.Add(em.Status.Name);
                }
            }
            return (IList<string>)selectedStatus;
        }
                
        public IList<string> selectedDepartment = new List<string>();
        public IList<string> GetDepartment(int SelectedEmployeeID)
        {
            if (selectedDepartment != null)
            {
                selectedDepartment.Clear();
                foreach (EmployeeModel em in globalEmployeeList.Where(person => person.ID == SelectedEmployeeID))
                {
                    selectedDepartment.Add(em.Department.Name);
                }
            }
            return (IList<string>)selectedDepartment;
        }

        public IList<string> selectedHireDate = new List<string>();
        public IList<string> GetHireDate(int SelectedEmployeeID)
        {
            if (selectedHireDate != null)
            {
                selectedHireDate.Clear();
                foreach (EmployeeModel em in globalEmployeeList.Where(person => person.ID == SelectedEmployeeID))
                {
                    selectedHireDate.Add(em.HireDate.ToString() );
                }
            }
            return (IList<string>)selectedHireDate;
        }
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Some quick remarks:

  • InitializeEmployeeList doesn't do what the method name says, since it also returns the globalEmployeeList. Why not name this a GetXXXX method?

  • Avoid using words like List or global in variable names or method names. A List<Employee> should simply be called Employees, for instance.

  • Be careful of what you make public.

  • I get why all these methods etc. are in this code-behind file, but IMHO you should attempt to move these as much as possible to one or more dedicated classes of their own. Consider that you've posted about 150 lines, and employeeGridView_CellClick isn't even 1/3 of those.

  • Use the proper collection types when you need them. for example, HashSet.

  • Do not pointlessly abbreviate: eModel is meaningless, just call it employeeModel. Ditto em, emm, etc..

  • I'd suggest to override Equals and GetHashCode in EmailModel and PhoneModel, that way the whole foreach (EmailModel emailModel in eModel.EmailList) etc. code can be replaced with a simple Linq command.

  • Parameters -- like SelectedEmployeeID-- need to be camelCase.

  • selectedStatus is not a correct name for a collection. Ditto selectedDepartment, etc.

  • Why do you do .DataSource = null; when in the next line you will fill the DataSource anyway?

  • Why do you do globalEmployeeList.ToList() when globalEmployeeList is already a List<T>?

\$\endgroup\$
7
  • \$\begingroup\$ Thanks for taking the time to comment. This is all very helpful for me. 1. I will rename InitializeEmployeeList(). That function's purpose evolved, while it's name didn't. 2. I will take the advice not to use List or global in variable names, but could you explain why? It seems to make the code more readable, but maybe that's just because I'm inexperienced; Employee and Employees look so similar that I feel it's more likely to lead to mistakes. 3. Okay, I will. \$\endgroup\$ Jan 28, 2021 at 14:47
  • \$\begingroup\$ 4. Yes! I was wondering if I should do that, and if so, how to structure it. Would you have a class called something like ListBoxDataSources where I keep that code, and then call those functions on the click event? 5. I have no idea what this means. I haven't discovered HashSet yet. I'll get there. 6. Okay, got it. 7. That seems a little over my head, but I think I get where you're coming from. 8. selectedStatus & selectedDepartment shouldn't be collections, but strings. I need to fix that. \$\endgroup\$ Jan 28, 2021 at 14:58
  • \$\begingroup\$ 9. Setting the datasource of the DataGridView to null before assigning it a value is a solution to a Winforms glitch that others have experienced. I mindlessly copied the code for the datasource of the DataGridView when setting the datasource of the listboxes. I need to correct that. 10. globalEmployeeList is an IEnumerable. I was probably thinking if I cast it to a list early on, I would avoid confusion later. One more question: is it a huge problem that I declare the employee list as static? I access it so frequently that it keeps me from passing it in repeatedly as a parameter. \$\endgroup\$ Jan 28, 2021 at 15:05
  • \$\begingroup\$ Naming things "List" or "global" etc. is "meta" information. It is a difficult naming convention to maintain, and Microsoft actively discourages it (what for instance when your List becomes an IEnumerable? Do you rename it?). To me, when I see something named "Employees", I know it is a collection. Whether it's an array or a List isn't that relevant to me. \$\endgroup\$
    – BCdotWEB
    Jan 28, 2021 at 15:10
  • \$\begingroup\$ Be careful to name something "ListBoxDataSources", because that way you're mixing UI and back-end. Even if you don't re-use that class anywhere else, it would IMHO be better to name it something more generic, e.g. EmployeeDataService. That the list of employees is static seems OK to me, but also look into making public properties readonly. \$\endgroup\$
    – BCdotWEB
    Jan 28, 2021 at 15:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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