Notifying Students in Courses – Strategically

I have the following implementation, and hope to get suggestions on how to improve the code.

• treeListViewModel.Courses is an observablecollection

• treeListViewModel.Courses[i].Students is a dictionary

The code loops as follows:

        List<uint> DataNotificationList = new List<uint>()
        {
            2,
            6,
            9
        };

        for (int n = 0; n < DataNotificationList.Count; n++)
        {
            for (int i = 0; i < treeListViewModel.Courses.Count; i++)
            {
                if (treeListViewModel.Courses[i].Students.ContainsKey(DataNotificationList[n]))
                {
                    treeListViewModel.Courses[i].Students[DataNotificationList[n]].ExecuteStrategy(hasError());
                    break;
                }
            }
        }

1. This smells of a message chain and violates the Law of Demeter (principle of least knowledge):

   treeListViewModel.Courses[i].Students[DataNotificationList[n]].ExecuteStrategy(hasError());
   

   Your caller very like doesn’t care of the exact internal workings of a course – it should just dispatch the notifications and see if it succeeded. So a Course should probably get a method like bool NotifyStudent(int studentId, bool hasError).

2. When you use expressions like DataNotificationList[n] in several places then it is usually better to put it in a local variable which expresses the meaning of it and also makes the code shorter, for example like this: int studentId = DataNotificationList[n];

3. If you don’t actually need the index then foreach will usually suffice and is shorter.

So in total your refactored code could look like this:

    foreach (int studentId in DataNotificationList)
    {
        foreach (Course course in treeListViewModel.Courses)
        {
            if (course.NotifyStudent(studentId, hasError()))
            {
                break;
            }
        }
    }

or if you want to go with a bit more LINQ:

    foreach (int studentId in DataNotificationList)
    {
        var course = treeListViewModel.Courses
                        .FirstOrDefault(c => c.NotifyStudent(studentId, hasError()));
        // could log something here and maybe raise an event handler
        // to say that a student in a course has been notified
    }

The NotifyStudent method in the course is responsible to check if the student exists in the course and return true once it has executed the strategy. It is totally encapsulated in the course and the caller doesn’t have to know about any internals.

There’s not much to go on here, but I can suggest a few things.

• I don’t see any benefit to using a List for DataNotificationList. A plain ol’ array would work just fine. Also, I would call it DataNotifications. Appending the word List to the end smells of reverse hungarian notation to me.

• I like that you’re using less than rather than Count - 1.

  for (int n = 0; n < DataNotificationList.Count; n++)
  

• It’s traditional to use i,j, and k as loop counters. Starting with i at the outermost level and working up the alphabet as you nest.