1
\$\begingroup\$

I have a class that produces a key based on the two objects passed. The key for the items passed is simply the string of the ID (a GUID) for each object. There is also a subclass that makes sure any to items are only represented once (sets).

The classes looked like the first code block below. But then I ran into a performance problem where creating the key every time was accessed was just to slow. So I changed it such that I create the key in the constructor and store it in a member variable. But now it seems that the inheritance is overkill.

Maybe the inheritance was always overkill but I didn't see it because I overrode an actual behavior so it seemed more OOPy? Or did it become inheritance abuse after the re-factoring? Or is it still fine but I am seeing it wrong?

Note that the ID must be unique and is often, but not always, a GUID.

Public Interface IIdentifiable
    ReadOnly Property ID() As String 
End Interface

Public Class Keyed
    Public Sub New(item1 As IIdentifiable, item2 As IIdentifiable)
        PrimaryItem = item1
        SecondaryItem = item2
    End Sub

    Public Property PrimaryItem As IIdentifiable

    Public Property SecondaryItem As IIdentifiable

    Public Overridable Function Key() As String
        Return PrimaryItem.ID & SecondaryItem.ID
    End Function

End Class

Public Class KeyedForSet
    Inherits Keyed

    Public Sub New(item1 As IIdentifiable, item2 As IIdentifiable)
        MyBase.New(item1, item2)
    End Sub

    Public Overrides Function Key() As String
        If PrimaryItem.ID.CompareTo(SecondaryItem.ID) > 0 Then
            Return PrimaryItem.ID & SecondaryItem.ID
        Else
            Return SecondaryItem.ID & PrimaryItem.ID
        End If
    End Function

End Class

Second implementation for performance reason.

Public Class Keyed
    Public Sub New(item1 As IIdentifiable, item2 As IIdentifiable)
        mKey = item1.ID & item2.ID
        PrimaryItem = item1
        SecondaryItem = item2
    End Sub

    Public Property PrimaryItem As IIdentifiable
    Public Property SecondaryItem As IIdentifiable

    Protected mKey as string
    Public Overridable Function Key() As String
        Return mKey
    End Function
End Class

Public Class KeyedForSet
    Inherits Keyed

    Public Sub New(item1 As IIdentifiable, item2 As IIdentifiable)
        MyBase.New(item1, item2)
        If PrimaryItem.ID.CompareTo(SecondaryItem.ID) > 0 Then
            mKey = PrimaryItem.ID & SecondaryItem.ID
        Else
            mKey = SecondaryItem.ID & PrimaryItem.ID
        End If
    End Sub

End Class
\$\endgroup\$
2
  • \$\begingroup\$ If you asked me, the inheritance is not a good idea here. You're not inheriting anything but a different implementation of the Key function. You should make a common interface and make both classes implement it. \$\endgroup\$ Jul 15, 2012 at 16:59
  • \$\begingroup\$ FYI - your changes in the second implementation changed functionality. Prior to the change if you assign a new object to either PrimaryItem or SecondaryItem the key changed automatically. Not so with the second implementation. \$\endgroup\$
    – nickles80
    Oct 13, 2012 at 19:18

2 Answers 2

1
\$\begingroup\$

Based upon your use case, the code below is what I would recommend. I'm sorry that it is in C#, on this computer I don't have my vb.net loaded so it was easier to express in c#.

There are 3 parts to this. The class on the bottom should address your needs (KeySet). the second class helps to make it more easily usable in your code (KeySetHelpers). The first class (MyExample) is just showing a few ways that you can utilize it. I think the direct approach is most natural.

Regarding performance and inheritance. If you follow the process below, I don't think you need another layer of inheritance. Performance wise, I think that the collection you choose to utilize will matter more than the KeySet. Since in the end you are just utilizing strings anyway.

One area I would recommend that you look into is the reason why you are flipping the primary/secondary keys based upon which is greater. Since you are already injecting which is the primary and which is secondary, they should most likely always have the same order when combined. Otherwise it could complicate the meaning of the key. Plus if you keep the order primary-secondary and normalize the key by stripping out anything non-alphanumeric and then adding in a parsing token like "-" between the too,...you wouldn't need to store the Key(composite), primary key, and secondary key. I hope this helps.

public class MyExample
{
    public void Test()
    {
        // Junk Prep
        var primary = System.Guid.NewGuid();
        var secondary = System.Guid.NewGuid();
        var junkData = "blah blah";

        // Real usage
        var keysList = new List<KeySet>();

        keysList.Add(new KeySet(primary, secondary)); // as guid
        // or
        keysList.Add(new KeySet(primary.ToString(), secondary.ToString())); // as guid strings
        // or
        keysList.Add(primary, secondary);// direct as guid


        var keysDictionary = new Dictionary<KeySet, object>();

        keysDictionary.Add(new KeySet(primary, secondary), junkData); // as guid
        // or            
        keysDictionary.Add(new KeySet(primary.ToString(), secondary.ToString()), junkData); // as guid strings
        // or
        keysDictionary.Add(primary, secondary, junkData); // direct as guid

        // etc.
    }
}

public static class KeySetHelpers
{
    #region Lists

    public static void Add(this List<KeySet> keyList, string primary, string secondary)
    {
        keyList.Add(new KeySet(primary, secondary));
    }

    public static void Add(this List<KeySet> keyList, Guid primary, Guid secondary)
    {
        keyList.Add(new KeySet(primary, secondary));
    }

    #endregion

    #region Dictionary

    public static void Add(this Dictionary<KeySet, object> keyDictionary, string primary, string secondary, object value)
    {
        keyDictionary.Add(new KeySet(primary, secondary), value);
    }

    public static void Add(this Dictionary<KeySet, object> keyDictionary, Guid primary, Guid secondary, object value)
    {
        keyDictionary.Add(new KeySet(primary, secondary), value);
    }

    #endregion
}



public class KeySet : IComparable<KeySet>
{
    public KeySet(Guid primary, Guid secondary) { EstablishKey(primary.ToString(), secondary.ToString()); }
    public KeySet(string primary, string secondary) { EstablishKey(primary, secondary); }

    #region Composite Key

    private void EstablishKey(string primary, string secondary)
    {
        if (string.Compare(primary, secondary) > 0)
        {
            Key = primary + secondary;
        }
        else
        {
            Key = secondary + primary;
        }

        Primary = primary;
        Secondary = secondary;
    }

    public string Primary { get; private set; }
    public string Secondary { get; private set; }

    public string Key { get; private set; }

    #endregion

    #region Functional Overrides

    public override bool Equals(object obj)
    {
        return (obj is KeySet) ? Equals((KeySet)obj) : false;
    }

    public bool Equals(KeySet obj)
    {
        return CompareTo(obj) == 0;
    }

    public override int GetHashCode()
    {
        return Key.GetHashCode();
    }

    public override string ToString()
    {
        return Key;
    }

    #endregion

    public int CompareTo(KeySet other)
    {
        return string.Compare(Key, other.Key);
    }

    public static bool operator ==(KeySet obj1, KeySet obj2)
    {
        return obj1.Equals(obj2);
    }

    public static bool operator !=(KeySet obj1, KeySet obj2)
    {
        return !obj1.Equals(obj2);
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Just something to add on. Per your statement "Note that the ID must be unique and is often, but not always, a GUID." Base upon your example, i would suggest normalizing to strings. Each new set of types you add on have to be hardened to ensure the object properly represents both the Equals() and GetHashCode() which can be hard if you frequently change root types. \$\endgroup\$
    – Randy
    Mar 17, 2012 at 2:05
0
\$\begingroup\$

A simple and quick solution is to cache you GUID generated in your original design. Ask the cache first for the GUID corresponding to inputs, if not there then generate it and cache it :) You can use Dictionary> for this, for key you can use any scheme like "id1:id2" string. (I assume ':' will not be part of an id ever, in case its false design your own scheme)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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