2
\$\begingroup\$

IMetadata

public interface IMetadata
{
    String ObjectFullName { get; set;  }
    String PropertyName { get; set; }
    String DisplayName { get; set; }
    String DisplayFormat { get; set; }
    Int32 DisplayOrder { get; set; }
}

public static class IMetadataExtensions
{
    public static String GetFullyQualifiedPropertyName(this IMetadata meta)
    {
        return String.Format("{0}.{1}", meta.ObjectFullName, meta.PropertyName);
    }
}

IMetadataContainer

/// <summary>
/// Contains metadata. Key will be either the PropertyName or the fully qualified PropertyName.
/// </summary>
public interface IMetadataContainer
{
    IDictionary<String, IMetadata> Metadata { get; set; }
}

IMetadataProvider

/// <summary>
/// Contains a collection of all available metadata.
/// </summary>
public interface IMetadataProvider
{
    IEnumerable<IMetadata> Metadata { get; set; }
}

public static class IMetadataProviderExtensions
{
    /// <summary>
    /// Injects the supplied metadata into the container.
    /// If there are conflicts, by default, the conflict will just replace the previous metadata information.
    /// To prevent this, you may fully qualify your property names with the object's full name by setting
    /// fullyQualifyOnConflict to 'true'.
    /// </summary>
    /// <returns>Where there any conflicts?</returns>
    public static Boolean InjectMetadata(this IMetadataProvider provider, IMetadataContainer container, String objectFullName, Boolean fullyQualifyOnConflict = false)
    {
        // VALIDATE: Null checks.
        if (container == null) { return true; }
        if (provider == null || provider.Metadata == null) { return true; }

        // EXECUTE: Iterate through all metadata grouped by their object adding them based on conflicts.
        Boolean conflictsExist = false; // This part of the alg might have a logical error or two, I haven't tested yet, I wanted feedback on the design first.
        IEnumerable<IMetadata> matchingMetadata = provider.Metadata.Where(m => m.ObjectFullName == objectFullName).GroupBy(m => m.ObjectFullName).Select(g => g.First());
        foreach (IMetadata meta in matchingMetadata)
        {
            Boolean conflictsExistOnObject = matchingMetadata.Where(m => m.ObjectFullName == meta.ObjectFullName).Any(m => container.Metadata.ContainsKey(m.PropertyName));
            conflictsExist = conflictsExist || conflictsExistOnObject;

            if (conflictsExistOnObject && fullyQualifyOnConflict)
            {
                container.Metadata.Add(meta.GetFullyQualifiedPropertyName(), meta);
            } 
            else if (conflictsExistOnObject && !fullyQualifyOnConflict)
            {
                container.Metadata.AddOrReplace(meta.PropertyName, meta);
            }
            else
            {
                container.Metadata.Add(meta.PropertyName, meta);
            }
        }

        // RETURN: Whether ANY conflicts were detected.
        return conflictsExist;
    }
}

MetadataWarehouse

/// <summary>
/// A Singleton class that manages the applications Metadata by caching it
/// instead of having to retrieve it from the database with every new HttpRequest.
/// </summary>
public sealed class MetadataWarehouse : IMetadataProvider
{
    /// <summary>
    /// A collection of all the Metadata in the database for this application.
    /// </summary>
    public IEnumerable<IMetadata> Metadata { get; set; }
    private EntityModels db;

    /// <summary>
    /// Gets the MetadataWarehouse Singleton.
    /// </summary>
    public static MetadataWarehouse Get
    {
        get
        {
            if (_Get == null) { _Get = new MetadataWarehouse(); }
            return _Get;
        }
    }
    private static MetadataWarehouse _Get;

    /// <summary>
    /// Private Constructor since this is a Singleton. 
    /// If anyone can create this class then it's not truly a Singleton.
    /// </summary>
    private MetadataWarehouse()
    {
        this.db = new EntityModels();
        _Refresh();
    }

    /// <summary>
    /// Refreshes the Metadata from the database.
    /// </summary>
    public static void Refresh()
    {
        MetadataWarehouse.Get._Refresh();
    }
    private void _Refresh()
    {
        this.Metadata = this.db.Metadatas.ToList();
    }
}

Usage

MetadataWarehouse is created at the start of my MVC application and retrieves all of the metadata on first use. I made it a singleton to prevent having to grab the same exact metadata nearly every single HttpRequest. So now with each request I just call

MetadataWarehouse.Get.InjectMetdata(container, "Namespace.ObjectName", true)

for each ObjectFullName that I need it for (which varies from 1-4 so far).

Concerns:

  1. Is this a good use for the Singleton? Is there another way to achieve this without a Singleton in ASP.NET MVC?

  2. Is it good/bad design to use Extension Methods on Interfaces like I have for InjectMetadata?

The only thing I'm not budging on is my usage of sectional comments (such as "// VALIDATE: Check nulls"). I find they help me improve readability of my code (directly and indirectly) and increase readability speed? (allows me and others to skim the code quicker)

\$\endgroup\$
  • \$\begingroup\$ Is this singleton supposed to persist data that is available to all users of the website? \$\endgroup\$ – dreza May 6 '15 at 20:36
  • \$\begingroup\$ @dreza Yes, The metadata stores DisplayNames, Formats, etc of Page elements. These don't change often at all but we don't want to have to push to production when they do change (which is why they're being stored in the database). \$\endgroup\$ – Shelby115 May 6 '15 at 20:43
2
\$\begingroup\$

Not a huge amount of code to review, just a few things:

  1. Get is a particular bad name for for the singleton property as Get typically implies an action rather than a property and such is expected to be a method. Traditionally static singleton instances are named Instance.

  2. The singleton instance is not thread-safe. Don't know about the rest of the code and how the startup works but it's possible for multiple requests coming in and obtaining two different instances. Even if it's currently not possible (because the instance is being accessed by the startup code from a single thread) you should still use Lazy<T> or lock it so you won't have to worry about it even if the code changes.

  3. The public static Refresh is sub-optimal since it's not clear what exactly it is refreshing - it's hiding the fact that it's refreshing the data of the singleton. I would get rid of it and just make the _Refresh method public (after renaming it to Refresh). It won't create much more typing work but be a lot more explicit about what exactly it is operating on.

  4. The main problem with static singleton instances is that they are prone to be accessed from everywhere - this creates implicit, invisible dependencies which can come easily back and bite you when it comes to unit testing and refactoring. So as much as possible classes which use it should be given an IMetadataProvider as a parameter to make it visible what they depend on.

    You should consider the use of an IoC container (Windsor, Ninject, Unity, ...) which greatly helps with this. Don't know out of the top of my head how it works in ASP.NET MVS but I'm sure it has support for resolving dependencies of controllers via IoC container.

  5. For extension methods it's customary to throw a ArgumentNullException when the this parameter is null. This brings it more in line with "normal" objects which would throw a NullReferenceException when they are null while being dereferenced.

  6. The GroupBy in here seems somewhat pointless:

    provider.Metadata.Where(m => m.ObjectFullName == objectFullName)
                     .GroupBy(m => m.ObjectFullName).Select(g => g.First());
    

    The Where already gives you all objects with matching objectFullName so you will only have one resulting group. You may as well take the first element after the Where and not bother grouping.

\$\endgroup\$
  • \$\begingroup\$ I did decide later to abandon this code as it was tightly coupled and was based on an uncertainty that metadata would change (it never did). As for your suggestions I definitely agree, I've learned a lot about DI/IoC since then. The groupBy I think might have just been a remnant of a refactor I did prior to posting this, not really sure what I was thinking there. \$\endgroup\$ – Shelby115 Jan 12 '16 at 20: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.