patterncsharpMinor
Windows Forms `ControlCollection` implementation
Viewed 0 times
implementationformscontrolcollectionwindows
Problem
I've implemented my own version of the
I've tried to make it thread-safe all around, but I'm sure I missed something.
My main concern is obviously the thread-safety of it, but I also would like any other comments as well.
The GitHub version as of this code is at: Control.ControlCollection.
```
using System;
using System.Collections;
using System.Collections.Generic;
using System.ComponentModel;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace Evbpc.Framework.Windows.Forms
{
public abstract partial class Control : Component, IComponent, IDisposable
{
///
/// Represents a collection of objects.
/// Is not actually inherited from ArrangedElementCollection. ArrangedElementCollection was removed from this implementation.
///
///
/// http://msdn.microsoft.com/en-us/library/system.windows.forms.control.controlcollection(v=vs.110).aspx
///
[ListBindable(false)]
public class ControlCollection : IEnumerable, ICloneable, IList, ICollection
{
private object _syncRoot = new object();
private object _internalSyncRoot = new object();
private List _controls;
private Control _owner;
#region Constructors
///
/// Initializes a new instance of the class.
///
/// A representing the control that owns the control collection.
public ControlCollection(Control owner)
{
_owner = owner;
_controls = new List();
}
#endregion
#region Properties
///
/// Gets the number of elements in the collection.
///
///
/// http://msdn.microsoft.com/en-us/library/system.windows.forms.layout.arrangedelementcollection.count(v=vs.1
Control.ControlCollection class in System.Windows.Forms and obviously I want it reviewed.I've tried to make it thread-safe all around, but I'm sure I missed something.
My main concern is obviously the thread-safety of it, but I also would like any other comments as well.
The GitHub version as of this code is at: Control.ControlCollection.
```
using System;
using System.Collections;
using System.Collections.Generic;
using System.ComponentModel;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace Evbpc.Framework.Windows.Forms
{
public abstract partial class Control : Component, IComponent, IDisposable
{
///
/// Represents a collection of objects.
/// Is not actually inherited from ArrangedElementCollection. ArrangedElementCollection was removed from this implementation.
///
///
/// http://msdn.microsoft.com/en-us/library/system.windows.forms.control.controlcollection(v=vs.110).aspx
///
[ListBindable(false)]
public class ControlCollection : IEnumerable, ICloneable, IList, ICollection
{
private object _syncRoot = new object();
private object _internalSyncRoot = new object();
private List _controls;
private Control _owner;
#region Constructors
///
/// Initializes a new instance of the class.
///
/// A representing the control that owns the control collection.
public ControlCollection(Control owner)
{
_owner = owner;
_controls = new List();
}
#endregion
#region Properties
///
/// Gets the number of elements in the collection.
///
///
/// http://msdn.microsoft.com/en-us/library/system.windows.forms.layout.arrangedelementcollection.count(v=vs.1
Solution
All your members should be
You don't want someone to set your lock members to
I have absolutely no explanation to this, but I have a guts feeling it'd be better if you threw the exception outside the
I'm no threading expert, ok? But this also feels wrong :
Everytime you call
Finally, I hate regions, you know that already :p
readonly! None of them should ever change.You don't want someone to set your lock members to
null. That would be bad, I don't think you possibly could change the parent Control, that would be weird, and I don't see the need to create a new instance of the list of child controls!lock (_internalSyncRoot)
{
foreach (Control c in _controls)
{
if (c.Name == key)
{
return c;
}
}
throw new KeyNotFoundException();
}I have absolutely no explanation to this, but I have a guts feeling it'd be better if you threw the exception outside the
lock statement. A lock should do as little as possible, so I think you should throw outside the lock.I'm no threading expert, ok? But this also feels wrong :
public virtual void AddRange(Control[] controls)
{
foreach (Control control in controls)
{
Add(control);
}
}Everytime you call
Add, you'll obtain a lock, then release the lock, then obtain the lock, etc. Consider obtaining a lock once and then calling InternalAdd in the loop. you'll avoid weird locking.Finally, I hate regions, you know that already :p
Code Snippets
lock (_internalSyncRoot)
{
foreach (Control c in _controls)
{
if (c.Name == key)
{
return c;
}
}
throw new KeyNotFoundException();
}public virtual void AddRange(Control[] controls)
{
foreach (Control control in controls)
{
Add(control);
}
}Context
StackExchange Code Review Q#109141, answer score: 4
Revisions (0)
No revisions yet.