Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  1. Using Equals(object, null) to test whether an object is null is rather unusual. I can't see any reason in your case why it would be preferred over object == null.

  2. Why is Parallel a nullable bool? I can't see any code which relies of the tri-state nature. If you insist on keeping it then a more concise way of checking it for true is if (Parallel == true) which will only evaluate to true if it is not null and true.

  3. Parallel is not a good name for a boolean flag. RunJobsInParallel is a bit longer but makes it much more clear what the flag is doing.

  4. Calling Abort() to terminate a thread is nasty Calling Abort() to terminate a thread is nasty. Use a flag which the main scheduler methods checks at convenient points in order to exit early. Use Join with a timeout to wait for the worker thread.

  5. _jobs is not thread safe. You iterate over it in ReviewJobs on the worker thread and you have public remove methods which could be called from a different thread to remove jobs from the list. This is waiting for an exception to happen ("collection was modified during enumeration").

  6. Actually upon reading your code more carefully: jobsToExecute is an IEnumerable and because it's built by a LINQ statement this means you "only" have a set of nested enumerators which iterate over the original _jobs collection when you do your foreach. This means your code should fail when you call _jobs.Remove() inside that loop. jobsToExecute needs to be built with ToList() or ToArray() to create a independent copy.

  7. If you want to wait for specific jobs to finish then you can put a ManualResetEvent on the Job class. This should be set by whoever executes the job once it is finished. Expose a WaitAsync() method on the Job class to wait on the event. Something along these lines:

     public class Job
     {
     ...
     private ManualResetEventSlim _JobDone = new ManualResetEventSlim(false);
     public void ProcessingStarted()
     {
     _JobDone.Reset();
     }
     public void ProcessingDone()
     {
     _JobDone.Set();
     }
     public async Task WaitAsync()
     {
     await Task.Run(() => _JobDone.WaitOne());
     }
     }
    

JobTrigger should do something along these lines:

 job.ProcessingStarted();
 ... // process job
 job.ProcessingDone();

Then something can wait on the job. As mentioned by svick the above code might not scale well if you have many jobs to wait on. Try StackOverflow if you need help solving this specific problem.

  1. Using Equals(object, null) to test whether an object is null is rather unusual. I can't see any reason in your case why it would be preferred over object == null.

  2. Why is Parallel a nullable bool? I can't see any code which relies of the tri-state nature. If you insist on keeping it then a more concise way of checking it for true is if (Parallel == true) which will only evaluate to true if it is not null and true.

  3. Parallel is not a good name for a boolean flag. RunJobsInParallel is a bit longer but makes it much more clear what the flag is doing.

  4. Calling Abort() to terminate a thread is nasty. Use a flag which the main scheduler methods checks at convenient points in order to exit early. Use Join with a timeout to wait for the worker thread.

  5. _jobs is not thread safe. You iterate over it in ReviewJobs on the worker thread and you have public remove methods which could be called from a different thread to remove jobs from the list. This is waiting for an exception to happen ("collection was modified during enumeration").

  6. Actually upon reading your code more carefully: jobsToExecute is an IEnumerable and because it's built by a LINQ statement this means you "only" have a set of nested enumerators which iterate over the original _jobs collection when you do your foreach. This means your code should fail when you call _jobs.Remove() inside that loop. jobsToExecute needs to be built with ToList() or ToArray() to create a independent copy.

  7. If you want to wait for specific jobs to finish then you can put a ManualResetEvent on the Job class. This should be set by whoever executes the job once it is finished. Expose a WaitAsync() method on the Job class to wait on the event. Something along these lines:

     public class Job
     {
     ...
     private ManualResetEventSlim _JobDone = new ManualResetEventSlim(false);
     public void ProcessingStarted()
     {
     _JobDone.Reset();
     }
     public void ProcessingDone()
     {
     _JobDone.Set();
     }
     public async Task WaitAsync()
     {
     await Task.Run(() => _JobDone.WaitOne());
     }
     }
    

JobTrigger should do something along these lines:

 job.ProcessingStarted();
 ... // process job
 job.ProcessingDone();

Then something can wait on the job. As mentioned by svick the above code might not scale well if you have many jobs to wait on. Try StackOverflow if you need help solving this specific problem.

  1. Using Equals(object, null) to test whether an object is null is rather unusual. I can't see any reason in your case why it would be preferred over object == null.

  2. Why is Parallel a nullable bool? I can't see any code which relies of the tri-state nature. If you insist on keeping it then a more concise way of checking it for true is if (Parallel == true) which will only evaluate to true if it is not null and true.

  3. Parallel is not a good name for a boolean flag. RunJobsInParallel is a bit longer but makes it much more clear what the flag is doing.

  4. Calling Abort() to terminate a thread is nasty. Use a flag which the main scheduler methods checks at convenient points in order to exit early. Use Join with a timeout to wait for the worker thread.

  5. _jobs is not thread safe. You iterate over it in ReviewJobs on the worker thread and you have public remove methods which could be called from a different thread to remove jobs from the list. This is waiting for an exception to happen ("collection was modified during enumeration").

  6. Actually upon reading your code more carefully: jobsToExecute is an IEnumerable and because it's built by a LINQ statement this means you "only" have a set of nested enumerators which iterate over the original _jobs collection when you do your foreach. This means your code should fail when you call _jobs.Remove() inside that loop. jobsToExecute needs to be built with ToList() or ToArray() to create a independent copy.

  7. If you want to wait for specific jobs to finish then you can put a ManualResetEvent on the Job class. This should be set by whoever executes the job once it is finished. Expose a WaitAsync() method on the Job class to wait on the event. Something along these lines:

     public class Job
     {
     ...
     private ManualResetEventSlim _JobDone = new ManualResetEventSlim(false);
     public void ProcessingStarted()
     {
     _JobDone.Reset();
     }
     public void ProcessingDone()
     {
     _JobDone.Set();
     }
     public async Task WaitAsync()
     {
     await Task.Run(() => _JobDone.WaitOne());
     }
     }
    

JobTrigger should do something along these lines:

 job.ProcessingStarted();
 ... // process job
 job.ProcessingDone();

Then something can wait on the job. As mentioned by svick the above code might not scale well if you have many jobs to wait on. Try StackOverflow if you need help solving this specific problem.

added more code and explanation
Source Link
ChrisWue
  • 20.6k
  • 4
  • 42
  • 107
  1. Using Equals(object, null) to test whether an object is null is rather unusual. I can't see any reason in your case why it would be preferred over object == null.

  2. Why is Parallel a nullable bool? I can't see any code which relies of the tri-state nature. If you insist on keeping it then a more concise way of checking it for true is if (Parallel == true) which will only evaluate to true if it is not null and true.

  3. Parallel is not a good name for a boolean flag. RunJobsInParallel is a bit longer but makes it much more clear what the flag is doing.

  4. Calling Abort() to terminate a thread is nasty. Use a flag which the main scheduler methods checks at convenient points in order to exit early. Use Join with a timeout to wait for the worker thread.

  5. _jobs is not thread safe. You iterate over it in ReviewJobs on the worker thread and you have public remove methods which could be called from a different thread to remove jobs from the list. This is waiting for an exception to happen ("collection was modified during enumeration").

  6. Actually upon reading your code more carefully: jobsToExecute is an IEnumerable and because it's built by a LINQ statement this means you "only" have a set of nested enumerators which iterate over the original _jobs collection when you do your foreach. This means your code should fail when you call _jobs.Remove() inside that loop. jobsToExecute needs to be built with ToList() or ToArray() to create a independent copy.

  7. If you want to wait for specific jobs to finish then you can put a ManualResetEvent on the Job class. This should be set by whoever executes the job once it is finished. Expose a WaitAsync() method on the Job class to wait on the event. Something along these lines:

     public class Job
     {
     ...
     private ManualResetEventSlim _JobDone = new ManualResetEventSlim(false);
     public void ProcessingStarted()
     {
     _JobDone.Reset();
     }
     public void ProcessingDone()
     {
     _JobDone.Set();
     }
     public async Task WaitAsync()
     {
     await Task.Run(() => _JobDone.WaitOne());
     }
     }
    

ProcessingStarted needs to be called by the JobTrigger which apparently executesshould do something along these lines:

 job.ProcessingStarted();
 ... // process job
 job.ProcessingDone();

Then something can wait on the tasksjob. As mentioned by svick the above code might not scale well if you have many jobs to wait on. Try StackOverflow if you need help solving this specific problem.

  1. Using Equals(object, null) to test whether an object is null is rather unusual. I can't see any reason in your case why it would be preferred over object == null.

  2. Why is Parallel a nullable bool? I can't see any code which relies of the tri-state nature. If you insist on keeping it then a more concise way of checking it for true is if (Parallel == true) which will only evaluate to true if it is not null and true.

  3. Parallel is not a good name for a boolean flag. RunJobsInParallel is a bit longer but makes it much more clear what the flag is doing.

  4. Calling Abort() to terminate a thread is nasty. Use a flag which the main scheduler methods checks at convenient points in order to exit early. Use Join with a timeout to wait for the worker thread.

  5. _jobs is not thread safe. You iterate over it in ReviewJobs on the worker thread and you have public remove methods which could be called from a different thread to remove jobs from the list. This is waiting for an exception to happen ("collection was modified during enumeration").

  6. Actually upon reading your code more carefully: jobsToExecute is an IEnumerable and because it's built by a LINQ statement this means you "only" have a set of nested enumerators which iterate over the original _jobs collection when you do your foreach. This means your code should fail when you call _jobs.Remove() inside that loop. jobsToExecute needs to be built with ToList() or ToArray() to create a independent copy.

  7. If you want to wait for specific jobs to finish then you can put a ManualResetEvent on the Job class. This should be set by whoever executes the job once it is finished. Expose a WaitAsync() method on the Job class to wait on the event. Something along these lines:

     public class Job
     {
     ...
     private ManualResetEventSlim _JobDone = new ManualResetEventSlim(false);
     public void ProcessingStarted()
     {
     _JobDone.Reset();
     }
     public async Task WaitAsync()
     {
     await Task.Run(() => _JobDone.WaitOne());
     }
     }
    

ProcessingStarted needs to be called by the JobTrigger which apparently executes the tasks.

  1. Using Equals(object, null) to test whether an object is null is rather unusual. I can't see any reason in your case why it would be preferred over object == null.

  2. Why is Parallel a nullable bool? I can't see any code which relies of the tri-state nature. If you insist on keeping it then a more concise way of checking it for true is if (Parallel == true) which will only evaluate to true if it is not null and true.

  3. Parallel is not a good name for a boolean flag. RunJobsInParallel is a bit longer but makes it much more clear what the flag is doing.

  4. Calling Abort() to terminate a thread is nasty. Use a flag which the main scheduler methods checks at convenient points in order to exit early. Use Join with a timeout to wait for the worker thread.

  5. _jobs is not thread safe. You iterate over it in ReviewJobs on the worker thread and you have public remove methods which could be called from a different thread to remove jobs from the list. This is waiting for an exception to happen ("collection was modified during enumeration").

  6. Actually upon reading your code more carefully: jobsToExecute is an IEnumerable and because it's built by a LINQ statement this means you "only" have a set of nested enumerators which iterate over the original _jobs collection when you do your foreach. This means your code should fail when you call _jobs.Remove() inside that loop. jobsToExecute needs to be built with ToList() or ToArray() to create a independent copy.

  7. If you want to wait for specific jobs to finish then you can put a ManualResetEvent on the Job class. This should be set by whoever executes the job once it is finished. Expose a WaitAsync() method on the Job class to wait on the event. Something along these lines:

     public class Job
     {
     ...
     private ManualResetEventSlim _JobDone = new ManualResetEventSlim(false);
     public void ProcessingStarted()
     {
     _JobDone.Reset();
     }
     public void ProcessingDone()
     {
     _JobDone.Set();
     }
     public async Task WaitAsync()
     {
     await Task.Run(() => _JobDone.WaitOne());
     }
     }
    

JobTrigger should do something along these lines:

 job.ProcessingStarted();
 ... // process job
 job.ProcessingDone();

Then something can wait on the job. As mentioned by svick the above code might not scale well if you have many jobs to wait on. Try StackOverflow if you need help solving this specific problem.

syntax error
Source Link
svick
  • 24.5k
  • 4
  • 53
  • 89
  1. Using Equals(object, null) to test whether an object is null is rather unusual. I can't see any reason in your case why it would be preferred over object == null.

  2. Why is Parallel a nullable bool? I can't see any code which relies of the tri-state nature. If you insist on keeping it then a more concise way of checking it for true is if (Parallel == true) which will only evaluate to true if it is not null and true.

  3. Parallel is not a good name for a boolean flag. RunJobsInParallel is a bit longer but makes it much more clear what the flag is doing.

  4. Calling Abort() to terminate a thread is nasty. Use a flag which the main scheduler methods checks at convenient points in order to exit early. Use Join with a timeout to wait for the worker thread.

  5. _jobs is not thread safe. You iterate over it in ReviewJobs on the worker thread and you have public remove methods which could be called from a different thread to remove jobs from the list. This is waiting for an exception to happen ("collection was modified during enumeration").

  6. Actually upon reading your code more carefully: jobsToExecute is an IEnumerable and because it's built by a LINQ statement this means you "only" have a set of nested enumerators which iterate over the original _jobs collection when you do your foreach. This means your code should fail when you call _jobs.Remove() inside that loop. jobsToExecute needs to be built with ToList() or ToArray() to create a independent copy.

  7. If you want to wait for specific jobs to finish then you can put a ManualResetEvent on the Job class. This should be set by whoever executes the job once it is finished. Expose a WaitAsync() method on the Job class to wait on the event. Something along these lines:

     public class Job
     {
     ...
     private ManualResetEventSlim _JobDone = new ManualResetEventSlim(false);
     public void ProcessingStarted()
     {
     _JobDone.Reset();
     }
     public async Task WaitAsync()
     {
     await Task.Run(() => _JobDone.WaitOne());
     }
     }
    

ProcessingStarted needs to be called by the JobTrigger which apparently executes the tasks.

  1. Using Equals(object, null) to test whether an object is null is rather unusual. I can't see any reason in your case why it would be preferred over object == null.

  2. Why is Parallel a nullable bool? I can't see any code which relies of the tri-state nature. If you insist on keeping it then a more concise way of checking it for true is if (Parallel == true) which will only evaluate to true if it is not null and true.

  3. Parallel is not a good name for a boolean flag. RunJobsInParallel is a bit longer but makes it much more clear what the flag is doing.

  4. Calling Abort() to terminate a thread is nasty. Use a flag which the main scheduler methods checks at convenient points in order to exit early. Use Join with a timeout to wait for the worker thread.

  5. _jobs is not thread safe. You iterate over it in ReviewJobs on the worker thread and you have public remove methods which could be called from a different thread to remove jobs from the list. This is waiting for an exception to happen ("collection was modified during enumeration").

  6. Actually upon reading your code more carefully: jobsToExecute is an IEnumerable and because it's built by a LINQ statement this means you "only" have a set of nested enumerators which iterate over the original _jobs collection when you do your foreach. This means your code should fail when you call _jobs.Remove() inside that loop. jobsToExecute needs to be built with ToList() or ToArray() to create a independent copy.

  7. If you want to wait for specific jobs to finish then you can put a ManualResetEvent on the Job class. This should be set by whoever executes the job once it is finished. Expose a WaitAsync() method on the Job class to wait on the event. Something along these lines:

     public class Job
     {
     ...
     private ManualResetEventSlim _JobDone = new ManualResetEventSlim(false);
     public void ProcessingStarted()
     {
     _JobDone.Reset();
     }
     public async WaitAsync()
     {
     await Task.Run(() => _JobDone.WaitOne());
     }
     }
    

ProcessingStarted needs to be called by the JobTrigger which apparently executes the tasks.

  1. Using Equals(object, null) to test whether an object is null is rather unusual. I can't see any reason in your case why it would be preferred over object == null.

  2. Why is Parallel a nullable bool? I can't see any code which relies of the tri-state nature. If you insist on keeping it then a more concise way of checking it for true is if (Parallel == true) which will only evaluate to true if it is not null and true.

  3. Parallel is not a good name for a boolean flag. RunJobsInParallel is a bit longer but makes it much more clear what the flag is doing.

  4. Calling Abort() to terminate a thread is nasty. Use a flag which the main scheduler methods checks at convenient points in order to exit early. Use Join with a timeout to wait for the worker thread.

  5. _jobs is not thread safe. You iterate over it in ReviewJobs on the worker thread and you have public remove methods which could be called from a different thread to remove jobs from the list. This is waiting for an exception to happen ("collection was modified during enumeration").

  6. Actually upon reading your code more carefully: jobsToExecute is an IEnumerable and because it's built by a LINQ statement this means you "only" have a set of nested enumerators which iterate over the original _jobs collection when you do your foreach. This means your code should fail when you call _jobs.Remove() inside that loop. jobsToExecute needs to be built with ToList() or ToArray() to create a independent copy.

  7. If you want to wait for specific jobs to finish then you can put a ManualResetEvent on the Job class. This should be set by whoever executes the job once it is finished. Expose a WaitAsync() method on the Job class to wait on the event. Something along these lines:

     public class Job
     {
     ...
     private ManualResetEventSlim _JobDone = new ManualResetEventSlim(false);
     public void ProcessingStarted()
     {
     _JobDone.Reset();
     }
     public async Task WaitAsync()
     {
     await Task.Run(() => _JobDone.WaitOne());
     }
     }
    

ProcessingStarted needs to be called by the JobTrigger which apparently executes the tasks.

expanded answer
Source Link
ChrisWue
  • 20.6k
  • 4
  • 42
  • 107
Loading
improved answer
Source Link
ChrisWue
  • 20.6k
  • 4
  • 42
  • 107
Loading
Source Link
ChrisWue
  • 20.6k
  • 4
  • 42
  • 107
Loading
lang-cs

AltStyle によって変換されたページ (->オリジナル) /