Answer 1

Hi,

You need to define what the comparison will do if x or y aren't of the type, T.  Try the following code  instead, for example.  No additional contracts  are required.

int IComparer.Compare(object x, object y)
{
 if (x is T && y is T)
 return Compare((T) x, (T) y);
 elsereturn -1;
}

- Dave

Answer 2

Hello Dave, thanks for the help!

However, using the "is" keyword to remove  unboxing warnings  changes the behavior of programs. For example, (string)null is valid, but null is string returns false. I tried a few combinations of if-else clauses and Contract.Assume statements to remove the warnings without changing behavior, but nothing works so far.

It seems to me that the original one-liner already succinctly expresses my intention to "pass the non-generic method call to the generic  method if arguments can be cast to the right types, else throw an exception to inform the caller of bad arguments". Just having trouble explaining this to the static checker.

Answer 3

Hi,

> [snip] else throw an exception to inform the caller of bad arguments

Edit: The problem actually isn't with the exception, because it turns out that's how Comparer<T> behaves as well.  The problem is that you are trying to do everything in one line.

Regardless, according to the documentation for IComparer<T>, you should be deriving from Comparer<T>anyway.  That way, you won't be responsible for implementing IComparer.

- Dave

Answer 4

Hi,

I just examined the source of the Comparer<T> class to see how the framework implements the non-generic version of IComparer.  Of course, it simply uses the is operator and has some null reference checks.  But it also throws an exception if the specified arguments aren't of the type, T.  If you really want to implement IComparer manually, then I suggest following the BCL's lead.

int IComparer.Compare(object x, object y)
{
  if (x == null)
  {
    if (y != null)
    {
      return -1;
    }
    return 0;
  }
  if (y == null)
  {
    return 1;
  }
  if ((x is T) && (y is T))
  {
    returnthis.Compare((T) x, (T) y);
  }
  ThrowHelper.ThrowArgumentException(ExceptionResource.Argument_InvalidArgumentForComparison);
  return 0;
}

- Dave

Answer 5

Thanks Dave, that's a very thorough and helpful investigation.

I used IComparer just to illustrate the problem more clearly, since it is a simple one-method interface. In practice, the problem arises often when one tries to implement a non-generic interface  with a generic  class, and have to propagate non-generic method calls to generic methods.

Nevertheless, your suggestion to follow the BCL's lead is the best solution in my opinion, and I'll be modifying my non-generic method definitions to include null-handling logic.

Here's a minor drawback to this pattern: whenever the null-handling logic changes in a generic method, one must remember to copy the updated code  to the corresponding non-generic method.

Answer 6

Hi,

> Here's a minor drawback to this pattern: whenever the null-handling logic changes in a generic  method,
> one must remember to copy the updated code  to the corresponding non-generic method.

True, but you could also move the logic into a reusable method instead of duplicating it.  For example:

private static int? CompareNull(object x, object  y)
{
 if (x == null)
 {
  if (y != null)
  {
    return  -1;
  }
  return 0;
 }
 if (y == null)
 {
  return 1;
 }
 return null;
}

Example usage:

int Compare(T x, T y)
{
 int? nullComparison = CompareNull(x, y);

 if (nullComparison.HasValue)
 {
 return nullComparison.Value;
 }
 elsereturn x.SomeProperty.CompareTo(y.SomeProperty);
}

int IComparer.Compare(object x, object y)
{
 int? nullComparison = CompareNull(x, y);

 if (nullComparison.HasValue)
 {
 return nullComparison.Value;
 }
 elseif ((x is T) && (y is T))
 {
 returnthis.Compare((T) x, (T) y);
 }
 ThrowHelper.ThrowArgumentException(ExceptionResource.Argument_InvalidArgumentForComparison);
 return 0;
}

- Dave

Answer 7

. . . you could also move the logic into a reusable method instead of duplicating it. . . .

Perfect. Thanks for pointing this out, Dave!

Answer 8

Hi,

I don't see a problem with your design.  What is the static checker warning  you about?

Edit: If it's warning you about unboxing  a null  reference from parameter within ICommand.CanExecute, then it's because you aren't actually following the pattern described above.  The null check in your example is in CanExecute.  My recommendation was to move it to a shared method and then call that method from both the explicit interface  implementation and also the typed implementation.

- Dave

Answer 9

Hi,

I don't see a problem with your design.  What is the static checker warning  you about?

Edit: If it's warning you about unboxing  a null  reference from parameter within ICommand.CanExecute, then it's because you aren't actually following the pattern described above.  The null check in your example is in CanExecute.  My recommendation was to move it to a shared method and then call that method from both the explicit interface  implementation and also the typed implementation.

- Dave

Answer 10

Here is the best solution I have so far. Doing all this just to remove  a couple of null  unboxing warnings  feels like overkill...

using System;
using System.Diagnostics.Contracts;
using System.Windows.Input;

publicclass DelegatedCommand<TActionParam, TPredicateParam> : ICommand
{
	readonly Action<TActionParam> _execute;
	readonly Predicate<TPredicateParam> _canExecute;
	readonly Action _deterministicExecute;
	readonly Func<bool> _deterministicCanExecute;

	publicevent EventHandler CanExecuteChanged
	{
		add { CommandManager.RequerySuggested += value; }
		remove { CommandManager.RequerySuggested -= value; }
	}

	public DelegatedCommand(
		Action<TActionParam> execute,
		Predicate<TPredicateParam> canExecute,
		Action deterministicExecute,
		Func<bool> deterministicCanExecute)
	{
		Contract.Requires(null != execute);

		_execute = execute;
		_canExecute = canExecute;
		_deterministicExecute = deterministicExecute;
		_deterministicCanExecute = deterministicCanExecute;
	}

	publicvoid Execute(TActionParam parameter)
	{
		_execute(parameter);
	}

	publicbool CanExecute(TPredicateParam parameter)
	{
		return (null == _canExecute) ? true : _canExecute(parameter);
	}

	void ICommand.Execute(object parameter)
	{
		if (null == parameter)
		{
			if (null == _deterministicExecute)
			{
				return;
			}
			else
			{
				_deterministicExecute();
				return;
			}
		}if (parameter is TActionParam)
		{
			Execute((TActionParam)parameter);
		}

		thrownew ArgumentException();
	}

	bool ICommand.CanExecute(object parameter)
	{
		if (null == parameter)
		{
			if (null == _deterministicCanExecute)
			{
				return true;
			}
			else
			{
				return _deterministicCanExecute();
			}
		}if (parameter is TPredicateParam)
		{
			return CanExecute((TPredicateParam)parameter);
		}

		thrownew ArgumentException();
	}

	[ContractInvariantMethod]
	void ObjectInvariant()
	{
		Contract.Invariant(null != _execute);
	}
}

Answer 11

Here is the best solution I have so far. Doing all this just to remove  a couple of null  unboxing warnings  feels like overkill...

using System;
using System.Diagnostics.Contracts;
using System.Windows.Input;

publicclass DelegatedCommand<TActionParam, TPredicateParam> : ICommand
{
	readonly Action<TActionParam> _execute;
	readonly Predicate<TPredicateParam> _canExecute;
	readonly Action _deterministicExecute;
	readonly Func<bool> _deterministicCanExecute;

	publicevent EventHandler CanExecuteChanged
	{
		add { CommandManager.RequerySuggested += value; }
		remove { CommandManager.RequerySuggested -= value; }
	}

	public DelegatedCommand(
		Action<TActionParam> execute,
		Predicate<TPredicateParam> canExecute,
		Action deterministicExecute,
		Func<bool> deterministicCanExecute)
	{
		Contract.Requires(null != execute);

		_execute = execute;
		_canExecute = canExecute;
		_deterministicExecute = deterministicExecute;
		_deterministicCanExecute = deterministicCanExecute;
	}

	publicvoid Execute(TActionParam parameter)
	{
		_execute(parameter);
	}

	publicbool CanExecute(TPredicateParam parameter)
	{
		return (null == _canExecute) ? true : _canExecute(parameter);
	}

	void ICommand.Execute(object parameter)
	{
		if (null == parameter)
		{
			if (null == _deterministicExecute)
			{
				return;
			}
			else
			{
				_deterministicExecute();
				return;
			}
		}if (parameter is TActionParam)
		{
			Execute((TActionParam)parameter);
		}

		thrownew ArgumentException();
	}

	bool ICommand.CanExecute(object parameter)
	{
		if (null == parameter)
		{
			if (null == _deterministicCanExecute)
			{
				return true;
			}
			else
			{
				return _deterministicCanExecute();
			}
		}if (parameter is TPredicateParam)
		{
			return CanExecute((TPredicateParam)parameter);
		}

		thrownew ArgumentException();
	}

	[ContractInvariantMethod]
	void ObjectInvariant()
	{
		Contract.Invariant(null != _execute);
	}
}

Answer 12

you can start using the easier-to-read (and perhaps more semantically correct) pattern of placing the null  constant as the second operand

Thanks for bringing this up. I will give it a try and see how it feels. (I had bad experiences with accidentally skipping the second = character before. As the proverb said, he who has been bitten by a snake will always be afraid of a rope...)

Also thanks for the code  suggestion. However, adding "Contract.Assume(parameter is TActionParam);" introduces the "is" keyword problem discussed earlier in this thread. For example, the problem would cause the last line of the following code to fail. I worked around it by adding the deterministic showNullText delegate, which also conforms to your suggestion of sharing the null-handling logic in one place.

Action showNullText = delegate() { MessageBox.Show(null); };

Action<string> showText = delegate(string text)
{
	if (text == null) showNullText();
	else MessageBox.Show(text);
};

DelegatedCommand<string, object> showTextCommand = new DelegatedCommand<string, object>(showText, null, showNullText, null);

ICommand command = showTextCommand;

showTextCommand.Execute("Hello");
command.Execute("Hello");
showTextCommand.Execute(null);
command.Execute(null);

Answer 13

Hi,

I assumed that you didn't want to pass null.  For example, if the parameter type is string, then null should still throw an exception.

Of course, if you do want to allow null then yes, you're going to have to work around it in a similar way as before.

- Dave

Answer 14

The following is a less intrusive way to remove  the null-unboxing warnings. It works in both the IComparer and ICommand cases discussed in this thread, as well as any other case in which an assumption needs to be made regarding the validity of a cast.

///<summary>/// Casts an object  to a specific type, assuming the cast is valid.///</summary>publicstatic T Cast<T>(thisobject x)
{
	object defaultReference = default(T);

	if (x == defaultReference)
	{
		returndefault(T);
	}
	else
	{
		Contract.Assume(x is T);

		return (T)x;
	}
}

Answer 15

Hi,

That's an interesting idea, although I wouldn't define it as an extension method on all objects.  Perhaps something like Cast.To<T>(obj) would be better.

- Dave

Answer 16