I have a class that represents an external physical measuring device. The simplified version looks like this:
public class Device {
public string Tag { get; set; }
public int Address { get; set; }
}
Tag is a user-defined value for identifying the device. Address is the value used by an adapter to communicate with the device. If two instances of Device have the same Address, then the same external measuring device will be used.
I'd like to mimic that behavior in code (for using methods like Contains and Distinct) by overriding Equals and implementing IEquatable<T>:
public class Device : IEquatable<Device> {
public string Tag { get; set; }
public int Address { get; set; }
public override bool Equals(object obj) {
return Equals(obj as Device);
}
public bool Equals(Device other) {
if (null == other) return false;
if (ReferenceEquals(this, other)) return true;
return Address.Equals(other.Address);
}
}
As you can see, I'm ignoring the Tag property in the implementation of Equals.
So, my question is: Should I ignore the Tag property in the implementation of Equals? Does doing so make the code harder to understand? Is there a better way of doing what I'm trying to do? I need the Tag property because, often, the user will not know the Address, or even whether or not the Device has an Address (that is taken care of in the App.config file, and the user will be dealing with an interface called IDevice which doesn't have an Address property).
Update:
Thanks everyone for the responses.
So, I gather that I should be using a custom IEqualityComparer. Do you have any guidance on how to do so if my real code looks more like this?
public interface IDevice {
string Tag { get; set; }
double TakeMeasurement();
}
internal class Device : IDevice {
public string Tag { get; set; }
public int Address { get; set; }
public double TakeMeasurement() {
// Take a measurement at the device's address...
}
}
Should I check the device type in my IEqualityComparer?
public class DeviceEqualityComparer : IEqualityComparer<IDevice> {
public bool Equals(IDevice x, IDevice y) {
Contract.Requires(x != null);
Contract.Requires(y != null);
if ((x is Device) && (y is Device)) {
return x.Address.Equals(y.Address);
}
else {
return x.Equals(y);
}
}
public int GetHashCode(IDevice obj) {
Contract.Requires(obj != null);
if (obj is Device) {
return obj.Address.GetHashCode();
}
else {
return obj.GetHashCode();
}
}
}
Yes, your current implementation is definitely confusing. The equality you've defined is clearly not the right notion of equality for devices.
So, rather than implementing IEquatable<Device> as you've done, I'd define an implementation of IEqualityComparer<Device>, maybe
class DeviceAddressEqualityComparer : IEqualityComparer<Device> {
public bool Equals(Device x, Device y) {
Contract.Requires(x != null);
Contract.Requires(y != null);
return x.Address.Equals(y.Address);
}
public int GetHashCode(Device obj) {
Contract.Requires(obj != null);
return obj.Address.GetHashCode();
}
}
You can pass instances of IEqualityComparer<T> to Contains, Distinct and other LINQ methods that depend on equality (e.g., GroupBy).
First of all you forgot to override GetHashCode(), so your code is broken.
IMO you should override Equals only if two objects are equivalent for all purposes. And objects with different Tags seem different for some purposes.
I wouldn't override Equals on these objects at all. I'd rather implement a custom comparer IEqualityComparer<T> and use that where appropriate. Most methods that have a notion of equality, take an IEqualityComparer<T> as optional parameter.
I wouldn't forbid null parameters, but handle them. I've also added an early-out for referential equality.
public class DeviceByAddressEqualityComparer : IEqualityComparer<IDevice> {
public bool Equals(IDevice x, IDevice y) {
if(x==y)
return true;
if(x==null||y==null)
return false;
return x.Address.Equals(y.Address);
}
public int GetHashCode(IDevice obj) {
if(obj == null)
return 0;
else
return obj.Address.GetHashCode();
}
}
If you want to check the type, depends on context. When overriding Equals I typically check with if x.GetType()==y.GetType(), but since you're using a special purpose comparer here, which deliberately ignores part of the object, I probably wouldn't make the type part of the identity.
Should I ignore the Tag property in the implementation of Equals?
No, I think this is a bad idea.
Does doing so make the code harder to understand?
Absolutely: a new developer would not understand how come two devices with different tags put in a hash set become one device.
Is there a better way of doing what I'm trying to do?
There are at least two ways that I can think:
- Provide a custom comparator
- Add a class called
DeviceWithTag, and keep the Device "tagless".
I would prefer the second approach, because it does look like your Tag models a real-world "tag" glued onto the device locator, which ignores its tag other than for display purposes.
Do you need to implement equality in terms of Tag at all? It doesn't sound like you do, so I don't see anything wrong with your approach.
If the user doesn't need to know about Address, then you might also argue that they don't need to know about the underlying equality based on address... do they? If they don't, then again, I would say there is nothing wrong with your approach.
If they do need to know about equality, then you may need to rethink your design and expose Address in some way.
I ended up creating a new interface for IDevice to implement. The new interface also lets me easily create equality comparers based on the device.
public interface IPhysicallyEquatable<T>
{
bool PhysicallyEquals(T other);
int GetPhysicalHashCode();
}
public class PhysicalEqualityComparer<T> : IEqualityComparer<T>
where T : IPhysicallyEquatable<T>
{
public bool Equals(T x, T y)
{
if (null == x) throw new ArgumentNullException("x");
if (null == y) throw new ArgumentNullException("y");
return x.PhysicallyEquals(y);
}
public int GetHashCode(T obj)
{
if (null == obj) throw new ArgumentNullException("obj");
return obj.GetPhysicalHashCode();
}
}
public interface IDevice : IPhysicallyEquatable<IDevice>
{
// ...
}
