5
\$\begingroup\$

I'm on a quest to combat all the magic strings which tends to pile up in an ASP.NET MVC application.

All reviews will be highly appreciate but please note that:

  • Some of the classes contains more (unrelated) code than I've presented here.
  • I do not use the c/c++ type aliases (unless forced to as with base type of an enum).
  • I use the this keyword for scoop readability. (Exception: razor syntax)

Examples

1) Getting an action URL in a view. (before/after)

Simple .NET Fiddle

<a href="@Url.Action("Match", "Users", new { name = Model.Name, age = Model.Age })">Similar users</a>

<a href="@(Url.Action<UsersController>((c) => c.Match(Model.Name, Model.Age)))">Similar users</a>

2) Performing an action redirect in controller. (before/after)

return this.RedirectToAction("Login", "Account", new { redirectUrl = url });

return this.RedirectToAction<AccountController>((c) => c.Login(url));

Code

MvcExtensions.cs

public static class MvcExtensions
{

    public static String Action<TController>(this UrlHelper urlHelper, Expression<Func<TController, ActionResult>> expression) where TController : Controller
    {

        if (urlHelper == null)
        {
            throw new ArgumentNullException(nameof(urlHelper));
        }
        else if (expression == null)
        {
            throw new ArgumentNullException(nameof(expression));
        }

        var info = ActionInfo.Create(expression);

        if (info.RouteValues.Count == 0)
        {
            return urlHelper.Action(info.ActionName, info.ControllerName);
        }

        return urlHelper.Action(info.ActionName, info.ControllerName, info.RouteValues);

    }

}

MvcController.cs

public class MvcController : Controller
{

    public MvcController()
    {
    }

    protected RedirectToRouteResult RedirectToAction<TController>(Expression<Func<TController, ActionResult>> expression) where TController : Controller
    {

        if (expression == null)
        {
            throw new ArgumentNullException(nameof(expression));
        }

        var info = ActionInfo.Create(expression);

        if (info.RouteValues.Count == 0)
        {
            return base.RedirectToAction(info.ActionName, info.ControllerName);
        }

        return base.RedirectToAction(info.ActionName, info.ControllerName, info.RouteValues);

    }

    protected RedirectToRouteResult RedirectToActionPermanent<TController>(Expression<Func<TController, ActionResult>> expression) where TController : Controller
    {

        if (expression == null)
        {
            throw new ArgumentNullException(nameof(expression));
        }

        var info = ActionInfo.Create(expression);

        if (info.RouteValues.Count == 0)
        {
            return base.RedirectToActionPermanent(info.ActionName, info.ControllerName);
        }

        return base.RedirectToActionPermanent(info.ActionName, info.ControllerName, info.RouteValues);

    }

}

ActionInfo.cs

internal sealed class ActionInfo
{

    private String actionName;
    private String controllerName;
    private RouteValueDictionary routeValues;

    private ActionInfo()
    {
    }

    public String ActionName
    {
        get
        {
            return this.actionName;
        }
    }

    public String ControllerName
    {
        get
        {
            return this.controllerName;
        }
    }

    public RouteValueDictionary RouteValues
    {
        get
        {
            return this.routeValues;
        }
    }

    internal static ActionInfo Create<TController>(Expression<Func<TController, ActionResult>> expression) where TController : Controller
    {

        if (expression == null)
        {
            throw new ArgumentNullException(nameof(expression));
        }

        var body = (MethodCallExpression)expression.Body;
        var routeValues = new RouteValueDictionary(Utils.GetMethodParameters(body));
        String actionName = Utils.GetActionNameFromMethod(body.Method);
        String controllerName = Utils.GetControllerNameFromType(typeof(TController));

        return new ActionInfo
        {
            actionName = actionName,
            controllerName = controllerName,
            routeValues = routeValues
        };

    }

}

Utils.cs

internal static class Utils
{

    internal const String ControllerNameSuffix = "Controller";

    internal static String GetActionNameFromMethod(MethodInfo method)
    {

        var attribute = method.GetCustomAttribute<ActionNameAttribute>(inherit: false);

        if ((attribute != null) && !String.IsNullOrEmpty(attribute.Name))
        {
            return attribute.Name;
        }

        return method.Name;

    }

    internal static String GetControllerNameFromType(Type type)
    {

        if ((type.Name.Length > ControllerNameSuffix.Length) && type.Name.EndsWith(ControllerNameSuffix))
        {
            return type.Name.Substring(0, (type.Name.Length - ControllerNameSuffix.Length));
        }

        return type.Name;

    }

    internal static Dictionary<String, Object> GetMethodParameters(MethodCallExpression expression)
    {

        var dictionary = new Dictionary<String, Object>();
        ParameterInfo[] parameters = expression.Method.GetParameters();

        for (Int32 index = 0; (index < parameters.Length); index++)
        {

            Expression argument = expression.Arguments[index];
            String name = parameters[index].Name;
            Object value = Expression.Lambda(Expression.Convert(argument, argument.Type)).Compile().DynamicInvoke();

            dictionary.Add(name, value);

        }

        return dictionary;

    }

}
\$\endgroup\$
  • \$\begingroup\$ Do you think you could add a method of the UserController? To see how your helpers deal with controllers? \$\endgroup\$ – IEatBagels Nov 13 '15 at 14:19
  • 1
    \$\begingroup\$ I've only implemented this in a test project. I need to ensure that it works as expected and I plan to do some performance tests. The UsersController inherits MvcController. The signature of the Match action is public ActionResult Match(String name, Int32 age). The method body is nothing more than two calls to ViewBag setting the parameters and finally return the view. The view prints out the value of each parameter. \$\endgroup\$ – Bjørn-Roger Kringsjå Nov 13 '15 at 14:39
  • 1
    \$\begingroup\$ Please add it! :) It's alright if it's a test controller in such a case, it's just to give a better overall view of the utilities! I'm writing an answer at the moment. \$\endgroup\$ – IEatBagels Nov 13 '15 at 14:42
7
\$\begingroup\$

First point, these extensions are really cool.

I don't have much comments to give, your code looks pretty good!

The ActionInfo class could be simplified a little :

internal sealed class ActionInfo
{
    private ActionInfo(string actionName, string controllerName, RouteValueDictionary routeValues)
    {
        ActionName = actionName;
        ControllerName = controllerName;
        RouteValues = routeValues;
    }

    public String ActionName { get; }
    public String ControllerName { get; }
    public RouteValueDictionary RouteValues { get; }

    internal static ActionInfo Create<TController>(Expression<Func<TController, ActionResult>> expression) where TController : Controller
    {
        if (expression == null)
        {
            throw new ArgumentNullException(nameof(expression));
        }

        var body = (MethodCallExpression)expression.Body;
        var routeValues = new RouteValueDictionary(Utils.GetMethodParameters(body));
        String actionName = Utils.GetActionNameFromMethod(body.Method);
        String controllerName = Utils.GetControllerNameFromType(typeof(TController));

        return new ActionInfo(actionName, controllerName, routeValues);
    }
}

Overall, you didn't need to members, you can use read-only properties from C#6.

You have a tendency for useless parenthesis! For example here :

if ((attribute != null) && !String.IsNullOrEmpty(attribute.Name))

(attribute != null) doesn't need parenthesis. They bulk the code and it serves no purpose.

Finally, this doesn't need the else, the code flow will never take advantage of the else if, a simple if would do the same work.:

if (urlHelper == null)
{
    throw new ArgumentNullException(nameof(urlHelper));
}
else if (expression == null)
{
    throw new ArgumentNullException(nameof(expression));
}
\$\endgroup\$
  • \$\begingroup\$ Monday, back to work. Thanks for your review! I'm not sure what you mean by "useless call to expressions". If you suggest me to use System.Convert then that won't work as Expression don't implement IConvertible. If you meant I should drop Expression.Lambda then I'm not sure how that would work either as Expression.Convert returns an unary (convert) expression. Can you please be more specific? :) \$\endgroup\$ – Bjørn-Roger Kringsjå Nov 16 '15 at 7:46

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.