6
\$\begingroup\$

I am trying to bridge the gap between the database context and routes that are stored in the database.

The factory is designed so that when used in other assemblies, you are forced to use the database context which is known to contain the DbSet which can provide the required data to populate the route.

In the assembly in which the factory resides, you can specify the database context to use so that if more route types are created the route factory can provide different types of route to other classes within the assembly.

However, the idea initially behind the factory was that the assemblies which reference the assembly with the factory could also specify a database context or model as long as it inherits the base database context or model. I'm aware that making the methods internal prevents this, but the methods have been made internal as they may be removed if I refactor the way everything works.

The following is the class:

namespace Sapphire.Cms.Web.Routing
{
    using Sapphire.Cms.Data.Entity;
    using Sapphire.Cms.Models;
    using System;
    using System.Web.Routing;

    public class RouteFactory : IRouteProvider
    {
        public virtual RouteBase GetRoute(IRouteHandler routeHandler)
        {
            DbContextTypeWrapper<SapphireDbContext<SiteTree>, SiteTree> dbContextTypeWrapper = new DbContextTypeWrapper<SapphireDbContext<SiteTree>, SiteTree>();
            return GetRoute(dbContextTypeWrapper, routeHandler);
        }

        internal virtual RouteBase GetRoute(DbContextTypeProvider databaseContextTypeProvider, IRouteHandler routeHandler)
        {
            return GetRoute(databaseContextTypeProvider, typeof(SiteTree), routeHandler);
        }

        internal virtual RouteBase GetRoute(DbContextTypeProvider databaseContextTypeProvider, Type model, IRouteHandler routeHandler)
        {
            Type catchallRoute = typeof(CatchallRoute<,>).MakeGenericType(new Type[] {
                databaseContextTypeProvider.DbContextType,
                model
            });

            Type routeTypeWrapper = typeof(RouteTypeWrapper<>).MakeGenericType(new Type[] {
                catchallRoute
            });

            RouteTypeProvider routeTypeProvider = (RouteTypeProvider)Activator.CreateInstance(routeTypeWrapper);

            return GetRoute(routeTypeProvider, routeHandler);
        }

        private static RouteBase GetRoute(RouteTypeProvider routeTypeProvider, IRouteHandler routeHandler)
        {
            Object[] constructorParameters = new Object[]
            {
                routeHandler
            };

            return (RouteBase)Activator.CreateInstance(routeTypeProvider.RouteType, constructorParameters);
        }

        #region IRouteProvider Members
        RouteBase IRouteProvider.GetRoute(IRouteHandler routeHandler)
        {
            return GetRoute(routeHandler);
        }
        #endregion
    }
}

As you can see the class makes heavy use of reflection, which is bad for performance, however the intended use of this factory is that it should only ever have to get one route, a catchall route.

My questions are:

  • Based on what was said, should I keep the internal methods internal or make them public?
  • Should the factory be refactored to improve performance despite the intention being to only ever use it once per request?
\$\endgroup\$
2
\$\begingroup\$

Any particular reason you have all those using statements in the namespace itself?


This line:

DbContextTypeWrapper<SapphireDbContext<SiteTree>, SiteTree> dbContextTypeWrapper = new DbContextTypeWrapper<SapphireDbContext<SiteTree>, SiteTree>();

throws a gray-flag for me. Perhaps you should consider injecting the DbContextTypeWrapper into the RouteFactory? It seems to have an awful dependence on certain specifics, that can be abstracted away.

Another suggestion, would be to consider generics for it, considering you also depend on SiteTree as well.

The ideal solution would be an interface ISiteTree that you depend on, regardless of the SiteTree implementation. You could provide a default SiteTree implementation, or allow the additional assemblies to provide their own.

Likewise, the use of internal tends to lend way to some particular properties, for example, the behavior between assemblies can be different. This is particularly foul. You should consider either marking the internal methods public, or marking them private.

Depending on what you have access to, you should consider interfaces/generics again to allow for the DbContextTypeWrapper to vary a little between assemblies. By restricting the public implementation to SapphireDbContext, you also restrict it to only those DB types that SapphireDbContext supports. (If it only supports MSSQL, then you'll need an MSSQL server for each-and-every other project you use this in.)


Though I do not generally advocate the use of var, you could shorten some of your lines with it. (Especially the obscenely long ones.)

\$\endgroup\$
  • \$\begingroup\$ Why don't you advocate the use of var? And I'm not sure what you were going for in the question regarding the using statements. Is there something you consider that the OP should have done differently there? \$\endgroup\$ – dreza Sep 11 '15 at 18:34
  • \$\begingroup\$ @dreza I'm just not a fan of var, does it really matter? It's quite handy here. Just as well, generally, using statements should be placed at the top of the file (not within the namespace itself). It's just a best-practice thing. (I would be somewhat surprised to see the using statements as the first declaration within the namespace, myself.) \$\endgroup\$ – 410_Gone Sep 11 '15 at 18:37
  • \$\begingroup\$ Ok, thanks for the further clarification. There are reasons to use namespaces within the using statements, but unless those are the reasons here then I agree with you on the generally taken approach of top of the file. \$\endgroup\$ – dreza Sep 11 '15 at 18:52
  • \$\begingroup\$ I used to put using inside the namespace as that's what I saw Microsoft doing in the .NET framework source, I don't bother now as it wastes time and Visual Studio gets glitchy when you do so. The framework has completely changed now so I use an n layer architecture and I know a fair bit more about unit testing now so I guess if I was to write this again it would look at lot different. \$\endgroup\$ – Matthew Bonner Sep 13 '15 at 15:19

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.