6
\$\begingroup\$

My logging adapter is using a dictionary for storing data that is passed from middleware to middleware.

It is a one-liner:

public class Log : Dictionary<SoftString, object> {}

Since it's using SoftString (like string but case-insensitive and trimmed) for the key and there are also two different types of data there, I was using a workaround by adding weird suffixes to the actual names to be able to tell them apart.

This system is very shaky so I'd like to have something stronger and easier to use. I thought I'll replace strings as tokens with some other type. This is how ItemKey<T> was born.

API

ItemKey<T> still uses a name but it now allows me to use T as a tag that I constrained to be an Enum. Both properties are used for the equality check.

public readonly struct ItemKey<T> : IEquatable<ItemKey<T>> where T : Enum
{
    public ItemKey(SoftString name, T tag)
    {
        Name = name;
        Tag = tag;
    }

    [AutoEqualityProperty]
    public SoftString Name { get; }

    [AutoEqualityProperty]
    public T Tag { get; }

    public override int GetHashCode() => AutoEquality<ItemKey<T>>.Comparer.GetHashCode(this);

    public override bool Equals(object obj) => obj is ItemKey<T> && Equals(obj);

    public bool Equals(ItemKey<T> other) => AutoEquality<ItemKey<T>>.Comparer.Equals(this, other);

    public static implicit operator ItemKey<T>((string name, T tag) key) => new ItemKey<T>(key.name, key.tag);
}

public enum LogItemTag
{
    Property, // These items are ready to be logged
    Metadata, // These items need to be processed before they can be a 'Property'
}

ItemKey<T> in action

From here I created the new Log dto with all the APIs that I need. I use only a single dictionary because I copy it in certain scenarios so it's easier to do it once rather than having a separate dictionary for each item type.

public class Log : IEnumerable<(ItemKey<LogItemTag> Key, object Value)>
{
    private readonly IDictionary<ItemKey<LogItemTag>, object> _data = new Dictionary<ItemKey<LogItemTag>, object>();

    public Log() { }

    private Log(IDictionary<ItemKey<LogItemTag>, object> data)
    {
        _data = new Dictionary<ItemKey<LogItemTag>, object>(data);
    }

    public static Log Empty => new Log();

    public Log Copy() => new Log(_data);

    public Log SetItem((string Name, LogItemTag Tag) key, object value)
    {
        _data[key] = value;
        return this;
    }

    public T GetItemOrDefault<T>((string Name, LogItemTag Tag) key, T defaultValue = default)
    {
        return _data.TryGetValue(key, out var obj) && obj is T value ? value : defaultValue;
    }

    public bool TryGetValue<T>((string Name, LogItemTag Tag) key, out T value)
    {
        if (_data.TryGetValue(key, out var obj) && obj is T result)
        {
            value = result;
            return true;
        }
        else
        {
            value = default;
            return false;
        }
    }

    public bool RemoveItem((string Name, LogItemTag Tag) key) => _data.Remove(key);

    public IEnumerator<(ItemKey<LogItemTag> Key, object Value)> GetEnumerator()
    {
        return _data.Select(x => (x.Key, x.Value)).GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

Extensions

I then extended it with a few helpers so that I don't always have to use the enum:

public static class LogExtensions
{
    public static Log SetProperty(this Log log, string name, object value)
    {
        return log.SetItem((name, LogItemTag.Property), value);
    }

    public static Log SetMetadata(this Log log, string name, object value)
    {
        return log.SetItem((name, LogItemTag.Metadata), value);
    }

    public static bool TryGetProperty<T>(this Log log, string name, out T value)
    {
        return log.TryGetValue((name, LogItemTag.Property), out value);
    }

    public static bool TryGetMetadata<T>(this Log log, string name, out T value)
    {
        return log.TryGetValue((name, LogItemTag.Metadata), out value);
    }
}

Questions

This is pretty simple but maybe you'll still have some more ideas how to enhance it?

\$\endgroup\$
  • \$\begingroup\$ Or should I make the tag a string? The enum cannot be extended by the user mhmmm.... \$\endgroup\$ – t3chb0t Aug 5 at 6:33
  • 1
    \$\begingroup\$ Is this intended to be used from multiple threads? \$\endgroup\$ – VisualMelon Aug 5 at 15:28
  • \$\begingroup\$ @VisualMelon nope, every call to logger.Log(..) will create a new instance of this Log dictionary. \$\endgroup\$ – t3chb0t Aug 5 at 15:30
2
\$\begingroup\$

I'm not sure what you gain by limiting this to using an enum as the Tag. I wouldn't use a string, that's for sure, but why not leave it completely free? If a user wants to use the LogItemTag schema, then they can. If they want to use strings, they are free to do so.


This look like an infinite loop (though hopefully it will StackOverflow):

public override bool Equals(object obj) => obj is ItemKey<T> && Equals(obj);

I presume this was the intention:

public override bool Equals(object obj) => obj is ItemKey<T> key && Equals(key);

I would prefer this was a method:

public static Log Empty => new Log();

It just feels wrong to be returning an Empty thing that isn't immutable.


Personally I would use KeyValuePair rather than a tuple for the key/value pairs, because I would never allow a tuple as part of a public API. Similarly, I would dispense with the tuple version of ItemKey in the APIs (e.g. on GetItemOrDefault): it's just adding inconsistency which will make the API harder to use. The implicit conversion seems like a fine way of 'overloading' the API (though again personally I wouldn't allow it).


Your clone constructor will initialise 2 dictionaries.


As always, the public API should carry inline documentation (///) so that it's purpose is clear to the maintainer and consumer. It doesn't need to be extensive or cref everything, but it should explain the intention clearly.


I'd expect Copy to be called Clone (copy sounds in-place to me). TryGetValue should be called TryGetItem to be consistent with the other methods.

\$\endgroup\$
  • \$\begingroup\$ Is there any particular reason you wouldn't allow tuples on public APIs or just a personal preference? The names of the fields are compiled, or rather decorated with an attribute so intellisense shows them. I find it's a convenient way of showing that these two names belong together. Oh boy, so many bugs :-\ I was lucky the equality check was strong so it didn't popup in my tests yet. \$\endgroup\$ – t3chb0t Aug 5 at 16:05
  • \$\begingroup\$ Mhmm... I think the copy constructor will create only one dictionary because the default constructor isn't called and the field is initialized by it. The assignment after field definition is just a syntactic sugar. I'm pretty sure this will be injected into the default ctor. ReSharper also isn't screaming but for the sake of consistency I'll better move it there myself. \$\endgroup\$ – t3chb0t Aug 5 at 16:07
  • 1
    \$\begingroup\$ @t3chb0t My hated of tuples is personal preference (I think most people would disagree that it's worth worrying about). The names do carry, but they can be splatted into another tuple with different names, so they are really a structural, rather than nominal, construct, and I do not like mixing structural and nominal constructs. The 'default constructor' is just a name for the parameterless constructor you get by default; it doesn't have any bearing on whether the fields are initialised or not. It has to call that twice, because the constructor could have side effects. \$\endgroup\$ – VisualMelon Aug 5 at 16:13
  • \$\begingroup\$ The default ctor will be called only when you do : this(), otherwise it won't be called, see example on ideone - I like the point about soft tuple names. You virtually convinced me to change it, I guess ;-P \$\endgroup\$ – t3chb0t Aug 5 at 16:18
  • \$\begingroup\$ @t3chb0t Aye, but the field initialisation is done before any constructor is called; it's not lumped into the 'default' ctor. (TIO example) \$\endgroup\$ – VisualMelon Aug 5 at 16:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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