HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Windows Forms `ControlCollection` implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationformscontrolcollectionwindows

Problem

I've implemented my own version of the 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 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.