Refactoring Into Generic Methods

Question

This might be a loaded question, but here goes. I have some code that is very similar between all the methods. I am trying to find a way to accomplish all methods with a generic method/function. I'm not sure if I should be using delegates, and if so how to implement in this specific situation, or if there is a much more simple way to make a generic method.

Below I have copied the code I am trying to make generic. All methods take this basic syntax: (Parameters) ConcurrentQueue(pPeople), With that collection, the first part gets the count of the records. Each method iterates over a different part of the collection (or collection in the collection).

Thanks for any help or nudges in the right direction! If you have an idea that would help, it would be great if you could give an example related to this code (or very similar).

Code: I have copied three methods that give that show similar code (there are many more).

 private void UpdatepPeopleAddressIDs(ConcurrentQueue<pPeople> people)
 {
 //Get Count of records that need an ID from uMaxIDs
 int updateCount = 0; object lockObject = new object();
 Parallel.ForEach<pPeople>(people, person =>
 {
 person.myPeopleAddressList.ForEach(address =>
 {
 if (address.ID == 0)
 {
 lock (lockObject)
 {
 updateCount++;
 }
 }
 });
 });
 }
 private void UpdatepPeopleContactNumbersIDs(ConcurrentQueue<pPeople> people)
 {
 //Get Count of records that need an ID from uMaxIDs
 int updateCount = 0; object lockObject = new object();
 Parallel.ForEach<pPeople>(people, person =>
 {
 person.myPeopleContactsList.ForEach(contact =>
 {
 if (contact.ID == 0)
 {
 lock (lockObject)
 {
 updateCount++;
 }
 }
 });
 });
 }
 private void UpdatepPeopleEmailIDs(ConcurrentQueue<pPeople> people)
 {
 //Get Count of records that need an ID from uMaxIDs
 int updateCount = 0; object lockObject = new object();
 Parallel.ForEach<pPeople>(people, person =>
 {
 person.myPeopleEmailsList.ForEach(emails =>
 {
 if (emails.ID == 0)
 {
 lock (lockObject)
 {
 updateCount++;
 }
 }
 });
 });
 }

• Marc Gravell: Observation: you are sharing a lock between all objects in that operation. Frankly, you might as well do it in series rather than parallel. An Interlocked.Increment might be a viable option. But again: since this is just a trivial test and increment, I can't see the purpose of parallelism here
• Marvin Smit: Observation2: Use Interlocked.Increment(ref this.counter); instead of full lock blocks.
• 大师 燈 Xi Huan: Do the address, contact and email class share a base class or interface? This would be easy if the ID property on those classes where defined in an interface...
• SolutionYogi: Agree with @MarcGravell. The code inside Parallel.ForEach is doing so little, you are better off not using Parallel.ForEach. ConcurrentQueue can change while you are enumerating over it (you get a copy) and the updateCount may or may not reflect the latest queue status. Finally, the most important thing: running these functions is USELESS. They update a local variable 'updateCount' and does nothing with it.
• mark: I simplified the methods dramatically. Stuff is done with updateCount, I just have left that out of my code since it is not relevant to this current question. I didn't want to paste hundreds of lines of code when all I need help on was one specific part of it. I do agree that parallel.foreach is fairly useless, I originally had more happening in that loop, but have changed as I have gone on. I realize some of the coding doesn't necessarily make sense since you can't see the whole solution, but it also isn't needed for this example.

Answer 1

This is fairly easy to achieve. You need to make use of callback, which can perform the functionality specific to the method, which your generic method then uses. Something like this:

private void UpdatepPeople<T>(ConcurrentQueue<pPeople> people,
 Func<PersonClass, List<T>> listProvider,
 Func<T, bool> updateRequiredTest)
{
 var updateCount = 0; 
 var lockObject = new object();
 Parallel.ForEach<pPeople>(people, person =>
 {
 listProvider(person).ForEach(item =>
 {
 if (updateRequiredTest(item))
 {
 lock (lockObject)
 {
 updateCount++;
 }
 }
 });
 });
}
private void UpdatepPeopleContactNumbersIDs(ConcurrentQueue<pPeople> people)
{
 UpdatepPeople<ContactType>(people,
 person => person.myPeopleContactsList,
 contact => contact.ID == 0);
}
private void UpdatepPeopleEmailIDs(ConcurrentQueue<pPeople> people)
{
 UpdatepPeople<EmailType>(people,
 person => person.myPeopleEmailsList,
 contact => emails.ID == 0);
}

Answer 2

Here is an example of an update people function and a call to it.

I make no claim to your code being correct, I'm just refactoring to the generic case given the code you posted.

 private void UpdatePeople(ConcurrentQueue<pPeople> people, Action<pPeople> actOn)
 {
 //Get Count of records that need an ID from uMaxIDs
 int updateCount = 0; object lockObject = new object();
 Parallel.ForEach<pPeople>(people, person => actOn(person));
 }
 private void UpdatepPeopleEmailIDs(ConcurrentQueue<pPeople> people)
 {
 UpdatePeople(people, person =>
 {
 person.myPeopleEmailsList.ForEach(emails =>
 {
 if (emails.ID == 0)
 {
 lock (lockObject)
 {
 updateCount++;
 }
 }
 });
 });
 }