This code loops through a GlobalEmployeesList, eliminates duplicate email and phone records for the selected employee, and populates listboxes

Question

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

Answer 1

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>?