Return to Answer

• 1

I will try to give you another review, once we made clear how it was designed and where is the biggest problem (in short: thread synchronization)

What I like

One note on style: The egregious use of regions and over-commenting is company style, I'm afraid, and I can't change it. Otherwise I'm looking for advice on style, efficiency, security, and particularly type safety.

Given the sample of the code:

/// <summary>
/// Waits for multiple concurrent operations to finish before firing a callback 
/// </summary>
public class TtConcurrentSynchronizer
{
 #region Properties
 private readonly List<Callback> callbacks = new List<Callback>();
 /// <summary>
 /// The callbacks that must all be executed before the finished function is fired. 
 /// </summary>
 public IEnumerable<Action> Callbacks
 {
 get { return callbacks.Select(x => CreateCallbackAction(x)); }
 }
 #endregion Properties
 #region Private Fields
 private Action finishedFunction;
 #endregion Private Fields

I would rather use classic Doxygen (or Javadoc) style, but we are in Microsoft's world - have to follow the rules. Those #region..#endregion are nice because you can collapse them. I usually add separating comments and indent them on one tab less:

/// <summary>
/// Waits for multiple concurrent operations to finish before firing a callback 
/// </summary>
public class TtConcurrentSynchronizer
{
//====================================== depends on your prefered width, e.g. 80 or 120
#region Properties
 private readonly List<Callback> callbacks = new List<Callback>();
 /// <summary>
 /// The callbacks that must all be executed before the finished function is fired. 
 /// </summary>
 public IEnumerable<Action> Callbacks
 {
 get { return callbacks.Select(x => CreateCallbackAction(x)); }
 }
#endregion Properties
//====================================== depends on your prefered width, e.g. 80 or 120
#region Private Fields
 private Action finishedFunction;
#endregion Private Fields

Because after collapsing you will see something like this:

+ #region Properties ...
 //=======================================
+ #region Private Fields ...

And those separating comments help with scrolling while the regions are expanded.

One day you will have to manage your code. Or worse, one day somebody else will have to manage your code. I am often not documenting my code enough when I am in a hurry and need to get the job done, but once there is the time for it, I clean and comment the code (especially when my stomach is full after a lunch and my brain refuses to work :D).

What I don't like

Pardon me, but the complexity, unclear interface and all those closures:

private Action CreateCallbackAction(Callback callback)
{
 return () =>
 {
 if (callbacks.Contains(callback))

Do you really need to check if callbacks.Contains(callback)? Who can remove it before it gets executed?

What I would like to see is one simple class (this is .NET not Java, I hate connecting small pieces in javax.swing - pardon me again if you like that). Your helper is fine in doing the job, but for me, it is internal, your helper and I want the final, easy to use class:

class WorkScheduler {
 public WorkScheduler() {...} // create it to fill the jobs later
 public WorkScheduler(Action finished, params Action[] jobs) // all-in-one
 public WorkScheduler(Action finished, params WorkAndFinishedPair[] jobs) // more complex
 public void Start() // to be used with default constructor
 public event Action FinalAction; // add the action if default-constructing
 public event Action Jobs; // add jobs by +=
 public void AddJob(Action job) // the same as above with +=
 public void AddJob(Action job, Action finished) // the more complex version

All I have seen in your question was using the synchronizer together with BackgroundWorkers. The above should be the interface to connect both together. If you want to add something else, create another class using your helper.

public void Start() {
 jobCounter = jobs.Count; // atomic/interlocked downcounter
 foreach(var pair in jobs) {
 var worker = new BackgroundWorker();
 worker.DoWork += pair.JobToDo; // with possible conversion
 worker.RunWorkerCompleted += pair.FinishAction != null ? ... : ...;
 worker.RunWorkerAsync(); // we can start it now,
 // because we have initialized the counter first
 }
}

You could as well use Threads instead of BackgroundWorkers but remember what you have learned about synchronization and ISynchronizeInvoke.BeginInvoke (you cannot acces Controls from different threads).