Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

Design

You appear to be creating events without actually using events. Why? If it looks like an event, smells like an event and quacks like an event, call it an event.

So:

private Action<object, PropertyChangedEventArgs> _CollectionItemChangedHandler;

Should be:

public event EventHandler<PropertyChangedEventArgs> _CollectionItemChangedHandler;

I can see you wanting to use Action to enforce that users are subscribed to both events and prevent unsubscription later on, but I wonder why you require this? If a user wants to create your class and not subscribe to these events, what does it matter?

It is also recommended not to use .ToList().ForEach() in that way.

You shouldn't use ForEach to change objects. LINQ should be used in a "functional" way (you can create new objects but you can't change old objects nor you can create side-effects). And what you are writing is creating so many useless List only to gain two lines of code...

See this StackOverflow answerthis StackOverflow answer for more information.

Design

You appear to be creating events without actually using events. Why? If it looks like an event, smells like an event and quacks like an event, call it an event.

So:

private Action<object, PropertyChangedEventArgs> _CollectionItemChangedHandler;

Should be:

public event EventHandler<PropertyChangedEventArgs> _CollectionItemChangedHandler;

I can see you wanting to use Action to enforce that users are subscribed to both events and prevent unsubscription later on, but I wonder why you require this? If a user wants to create your class and not subscribe to these events, what does it matter?

It is also recommended not to use .ToList().ForEach() in that way.

You shouldn't use ForEach to change objects. LINQ should be used in a "functional" way (you can create new objects but you can't change old objects nor you can create side-effects). And what you are writing is creating so many useless List only to gain two lines of code...

See this StackOverflow answer for more information.

Design

You appear to be creating events without actually using events. Why? If it looks like an event, smells like an event and quacks like an event, call it an event.

So:

private Action<object, PropertyChangedEventArgs> _CollectionItemChangedHandler;

Should be:

public event EventHandler<PropertyChangedEventArgs> _CollectionItemChangedHandler;

I can see you wanting to use Action to enforce that users are subscribed to both events and prevent unsubscription later on, but I wonder why you require this? If a user wants to create your class and not subscribe to these events, what does it matter?

It is also recommended not to use .ToList().ForEach() in that way.

You shouldn't use ForEach to change objects. LINQ should be used in a "functional" way (you can create new objects but you can't change old objects nor you can create side-effects). And what you are writing is creating so many useless List only to gain two lines of code...

See this StackOverflow answer for more information.

Removed unsubstantiated style point
Source Link
Nick Udell
  • 5.2k
  • 1
  • 28
  • 68

Style

Use camel casing for private non-constant fields.

So:

private bool _freezeNotifications;

Should be:

private bool _freezeNotifications;

(I also don't like the underscore notation, but that's more of a personal choice).

Design

You appear to be creating events without actually using events. Why? If it looks like an event, smells like an event and quacks like an event, call it an event.

So:

private Action<object, PropertyChangedEventArgs> _CollectionItemChangedHandler;

Should be:

public event EventHandler<PropertyChangedEventArgs> _CollectionItemChangedHandler;

I can see you wanting to use Action to enforce that users are subscribed to both events and prevent unsubscription later on, but I wonder why you require this? If a user wants to create your class and not subscribe to these events, what does it matter?

It is also recommended not to use .ToList().ForEach() in that way.

You shouldn't use ForEach to change objects. LINQ should be used in a "functional" way (you can create new objects but you can't change old objects nor you can create side-effects). And what you are writing is creating so many useless List only to gain two lines of code...

See this StackOverflow answer for more information.

Style

Use camel casing for private non-constant fields.

So:

private bool _freezeNotifications;

Should be:

private bool _freezeNotifications;

(I also don't like the underscore notation, but that's more of a personal choice).

Design

You appear to be creating events without actually using events. Why? If it looks like an event, smells like an event and quacks like an event, call it an event.

So:

private Action<object, PropertyChangedEventArgs> _CollectionItemChangedHandler;

Should be:

public event EventHandler<PropertyChangedEventArgs> _CollectionItemChangedHandler;

I can see you wanting to use Action to enforce that users are subscribed to both events and prevent unsubscription later on, but I wonder why you require this? If a user wants to create your class and not subscribe to these events, what does it matter?

It is also recommended not to use .ToList().ForEach() in that way.

You shouldn't use ForEach to change objects. LINQ should be used in a "functional" way (you can create new objects but you can't change old objects nor you can create side-effects). And what you are writing is creating so many useless List only to gain two lines of code...

See this StackOverflow answer for more information.

Design

You appear to be creating events without actually using events. Why? If it looks like an event, smells like an event and quacks like an event, call it an event.

So:

private Action<object, PropertyChangedEventArgs> _CollectionItemChangedHandler;

Should be:

public event EventHandler<PropertyChangedEventArgs> _CollectionItemChangedHandler;

I can see you wanting to use Action to enforce that users are subscribed to both events and prevent unsubscription later on, but I wonder why you require this? If a user wants to create your class and not subscribe to these events, what does it matter?

It is also recommended not to use .ToList().ForEach() in that way.

You shouldn't use ForEach to change objects. LINQ should be used in a "functional" way (you can create new objects but you can't change old objects nor you can create side-effects). And what you are writing is creating so many useless List only to gain two lines of code...

See this StackOverflow answer for more information.

Source Link
Nick Udell
  • 5.2k
  • 1
  • 28
  • 68

Style

Use camel casing for private non-constant fields.

So:

private bool _freezeNotifications;

Should be:

private bool _freezeNotifications;

(I also don't like the underscore notation, but that's more of a personal choice).

Design

You appear to be creating events without actually using events. Why? If it looks like an event, smells like an event and quacks like an event, call it an event.

So:

private Action<object, PropertyChangedEventArgs> _CollectionItemChangedHandler;

Should be:

public event EventHandler<PropertyChangedEventArgs> _CollectionItemChangedHandler;

I can see you wanting to use Action to enforce that users are subscribed to both events and prevent unsubscription later on, but I wonder why you require this? If a user wants to create your class and not subscribe to these events, what does it matter?

It is also recommended not to use .ToList().ForEach() in that way.

You shouldn't use ForEach to change objects. LINQ should be used in a "functional" way (you can create new objects but you can't change old objects nor you can create side-effects). And what you are writing is creating so many useless List only to gain two lines of code...

See this StackOverflow answer for more information.