I have N child classes of the same base class. The base contains abstract methods to convert the class to a byte array and vice versa.

It also contains a method to compare that instance to two other instances of the same type. Result shall be true if this instance is within the range of the other two instances (each property value is within the range of the corresponding property values).

Collections shall be compared by comparing each value.

Because there are N childs I really don't want to implement the compare method in each child (huge amount of testing and boring code) so I use a property bag in the base class and implement the compare method there.

Child property implementation:

    public int IntValue
    {
        get { return m_IntValue; }
        set
        {
            m_IntValue = value;
            AddOrUpdateProperty(value);
        }
    }
    public string StringValue
    {
        get { return m_StringValue; }
        set
        {
            m_StringValue = value;
            AddOrUpdateProperty((IComparable)value);
        }
    }
    public byte[] ByteArray
    {
        get { return m_ByteArray; }
        set
        {
            m_ByteArray = value;
            AddOrUpdateProperty(value);
        }
    }

Methods to add or update the property bag Dictionary<string, object> m_PropertyBag. Only properties that implement IComparable are allowed.

private Dictionary<string, object> m_PropertyBag = new Dictionary<string, object>();

protected void AddOrUpdateProperty(IComparable value, [CallerMemberName]string name = null)
{
    AddOrUpdateProperty(value, name);
}

protected void AddOrUpdateProperty<T>(IEnumerable<T> value, [CallerMemberName]string name = null)
    where T : IComparable
{
    AddOrUpdateProperty(value, name);
}

private void AddOrUpdateProperty(object value, string name)
{
    if (value == null) { throw new ArgumentNullException("value"); }
    if (name == null) { throw new ArgumentNullException("name"); }
    if (name == string.Empty) { throw new ArgumentException("Name must not be empty.", "name"); }
    if (name.Contains(" ")) { throw new ArgumentException("Name must not contain any white spaces.", "name"); }

    if (m_PropertyBag.ContainsKey(name)) { m_PropertyBag[name] = value; }
    else { m_PropertyBag.Add(name, value); }
}

Here is the compare method for the whole bag.

public bool Compare(Base min, Base max)
{
    if (GetType() != min.GetType() || GetType() != max.GetType())
            throw new InvalidOperationException("Not the same type.");

    var result = true;
    foreach (var property in m_PropertyBag)
    {
        if (property.Value is IEnumerable)
        {
            var values = (IEnumerable)property.Value;
            var minValuesEnum = ((IEnumerable)min.m_PropertyBag[property.Key]).GetEnumerator();
            var maxValuesEnum = ((IEnumerable)max.m_PropertyBag[property.Key]).GetEnumerator();

            foreach (var value in values)
            {
                result &= minValuesEnum.MoveNext();
                result &= maxValuesEnum.MoveNext();
                if (!result) return false;

                result &= Compare(value, minValuesEnum.Current, maxValuesEnum.Current);
                if (!result) return false;
            }
        }
        else
        {
            result &= Compare(property.Value, min.m_PropertyBag[property.Key], max.m_PropertyBag[property.Key]);
            if (!result) return false;
        }
    }
    return result;
}

Here is the compare method for a single IComparable property.

public bool Compare(object value, object min, object max)
{
    var comparableObject = (IComparable)value;
    return comparableObject.CompareTo(min) >= 0 && comparableObject.CompareTo(min) <= 0;
}
  • Ok, cool. Thanks for sharing this info. Now, what's the question? :) – Gentian Kasa May 17 '15 at 9:54
  • In other words, do you have any particular concerns with the code or are you just looking for a general review? – RubberDuck May 17 '15 at 10:39
  • Well I completely forgot to mention that I'm not very familiar with objects. I try to avoid them using generics. But in this case I guess objects are my best bet. So it would be great to know I've that code is ok and if this design is acceptable. I know that there is a high performance impact but I my case the compare operation is done max 50 times / sec mean 5 times / sec. So the performance impact overall is insignificant. – nJob May 17 '15 at 18:05
up vote 4 down vote accepted

It is a good idea to use consistent styles throughout your code. Right here, you use braces around a 1-line if/else block:

if (m_PropertyBag.ContainsKey(name)) { m_PropertyBag[name] = value; }
else { m_PropertyBag.Add(name, value); }

Right here, you do not:

if (GetType() != min.GetType() || GetType() != max.GetType())
        throw new InvalidOperationException("Not the same type.");

I recently switched to always using braces, but whatever you decide, be consistent.


I don't think you need all those assignments to result when you are just going to return if the value is false:

var result = true;
foreach (var property in m_PropertyBag)
{
    if (property.Value is IEnumerable)
    {
        var values = (IEnumerable)property.Value;
        var minValuesEnum = ((IEnumerable)min.m_PropertyBag[property.Key]).GetEnumerator();
        var maxValuesEnum = ((IEnumerable)max.m_PropertyBag[property.Key]).GetEnumerator();

        foreach (var value in values)
        {
            result &= minValuesEnum.MoveNext();
            result &= maxValuesEnum.MoveNext();
            if (!result) return false;

            result &= Compare(value, minValuesEnum.Current, maxValuesEnum.Current);
            if (!result) return false;
        }
    }
    else
    {
        result &= Compare(property.Value, min.m_PropertyBag[property.Key], max.m_PropertyBag[property.Key]);
        if (!result) return false;
    }
}
return result;

You can do this instead:

foreach (var property in m_PropertyBag)
{
    if (property.Value is IEnumerable)
    {
        var values = (IEnumerable)property.Value;
        var minValuesEnum = ((IEnumerable)min.m_PropertyBag[property.Key]).GetEnumerator();
        var maxValuesEnum = ((IEnumerable)max.m_PropertyBag[property.Key]).GetEnumerator();

        foreach (var value in values)
        {
            if (!minValuesEnum.MoveNext() ||
                !maxValuesEnum.MoveNext() ||
                !Compare(value, minValuesEnum.Current, maxValuesEnum.Current);
            {
                return false;
            }
        }
    }
    else if (!Compare(property.Value, min.m_PropertyBag[property.Key], max.m_PropertyBag[property.Key]))
    {
        return false;
    }
}
return true;

I'm not sure what is going to, or should, happen if neither the if nor the else if statements execute. Should there be an else { return false; } statement in there?

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.