Monkeying around with Survey Monkey and Asp.Net Mvc

Question

Intro

I've been a desktop dev for a long time now, and have never really had to monkey with web development until very recently. I have a need to do some custom integration with Survey Monkey's Api and as a "Hello World" I came up with this simple ASP.Net MVC app that queries Survey Monkey for a list of my surveys and displays some real basic info about them. (Nothing I couldn't get from just going to their site, but hey!, I'm learning here.)

I have no idea what I'm doing, so any and all feedback would be greatly appreciated. I'd be particularly interested in some way to cache the results from the API request so that I don't have to fetch it each time the page loads.

The Tech

• C# 6.0
• MVC 5.0
• Ninject MVC for dependency injection.
• Survey Monkey API wrapper for C#.

The Code

The API wrapper library needs an API Key & Token to connect to Survey Monkey. In order to keep them out of the GitHub repo, I created some environment variables and a factory class that knows how to access them and return an instance of SurveyMonkeyApi.

./SurveyMonkeyApiFactory.cs

namespace SurveyMonkeyPlayground
{
    public interface ISurveyMonkeyApiFactory
    {
        SurveyMonkeyApi Create();
    }

    public class SurveyMonkeyApiFactory : ISurveyMonkeyApiFactory
    {
        public SurveyMonkeyApi Create()
        {
            string apiKey = Environment.GetEnvironmentVariable("SM_APIKEY");
            string token = Environment.GetEnvironmentVariable("SM_TOKEN");

            return new SurveyMonkeyApi(apiKey, token);
        }
    }
}

Next I created a model that is a subset of the full Survey object from the 3rd party library.

./Models/SurveyModel.cs

namespace SurveyMonkeyPlayground.Models
{
    public class SurveyModel
    {
        public string Name { get; set; }
        public int ResponseCount { get; set; }
        public string Url { get; set; }
    }
}

And then a controller to handle requests for a list of all surveys, and details about any given survey.

./Controllers/SurveysController.cs

namespace SurveyMonkeyPlayground.Controllers
{
    public class SurveysController : Controller
    {
        private ISurveyMonkeyApiFactory _apiFactory;
        public SurveysController(ISurveyMonkeyApiFactory factory)
        {
            _apiFactory = factory;
        }

        // GET: Surveys
        public ActionResult Index()
        {
            var surveyMonkey = _apiFactory.Create();
            var surveys = surveyMonkey.GetSurveyList()
                            .Select(s => new SurveyModel() { Name = s.Nickname, ResponseCount = s.NumResponses, Url = s.AnalysisUrl });

            return View(surveys);
        }

        // GET: Surveys/Details?name=SomeSurveyName
        public ActionResult Details(string name)
        {
            var surveyMonkey = _apiFactory.Create();
            var survey = surveyMonkey.GetSurveyList()
                            .Where(s => s.Nickname == name)
                            .Select(s => new SurveyModel() { Name = s.Nickname, ResponseCount = s.NumResponses, Url = s.AnalysisUrl })
                            .First();

            return View(survey);
        }
    }
}

And finally, some views to display this all.

./Views/Surveys/Index.cshtml

@model IEnumerable<SurveyMonkeyPlayground.Models.SurveyModel>

@{
    ViewBag.Title = "Surveys";
}

<h2>Surveys</h2>

<table class="table">
    <tr>
        <th>
            @Html.DisplayNameFor(model => model.Name)
        </th>
        <th>
            @Html.DisplayNameFor(model => model.ResponseCount)
        </th>
        <th></th>
    </tr>

@foreach (var item in Model) {
    <tr>
        <td>
            <a href="@item.Url">@item.Name</a>
        </td>
        <td>
            @Html.DisplayFor(modelItem => item.ResponseCount)
        </td>
        <td>
            @Html.ActionLink("Details", "Details", new { Name = item.Name})
        </td>
    </tr>
}

</table>

./Views/Surveys/Details.cshtml

@model SurveyMonkeyPlayground.Models.SurveyModel

@{
    ViewBag.Title = "Surveys";
}

<h2>@Model.Name Details</h2>

<div>
    <hr />
    <dl class="dl-horizontal">
        <dt>
            @Html.DisplayNameFor(model => model.Name)
        </dt>

        <dd>
            @Html.DisplayFor(model => model.Name)
        </dd>

        <dt>
            @Html.DisplayNameFor(model => model.ResponseCount)
        </dt>

        <dd>
            @Html.DisplayFor(model => model.ResponseCount)
        </dd>

        <dt>
            Result Analysis Url
        </dt>

        <dd>
            <a href="@Html.DisplayFor(model => model.Url)">@Html.DisplayFor(model => model.Url)</a>
        </dd>

    </dl>
</div>
<p>
    @Html.ActionLink("Back to List", "Index")
</p>

Answer 1

This looks like a typo to me:

<dd>
   <a href="@Html.DisplayFor(model => model.Url)">@Html.DisplayFor(model => model.Url)</a>
</dd>

Shouldn't it just be:

<dd>
   <a href="@Model.Url">@Html.DisplayFor(model => model.Url)</a>
</dd>

One thing I find really useful when dealing with projecting to model classes is either use automapper, or write an extension method:

public static class SurveyExtensions
{
    public IEnumerable<SurveyModel> ProjectToSurveyModel(IEnumerable<SurveyMonkey.Survey> this source)
    {
         // null check
         return source.Select(s => new SurveyModel 
             { 
                 Name = s.Nickname, 
                 ResponseCount = s.NumResponses, 
                 Url = s.AnalysisUrl 
             });
    }
}

Then you can simplify your controller:

// [Route("Surveys")]
public ActionResult Index()
{
    var surveyMonkey = _apiFactory.Create();
    var surveys = surveyMonkey.GetSurveyList()
                    .ProjectToSurveyModel();
    return View(surveys);
}

I've added in a commented out Route attribute just in case that floats your boat duck. I personally prefer attribute routing because I like to see the url in place. Especially because one of the people I work with is mad for clean urls, so you can do:

[Route("something-important/{id}/collection-of-stuff/{collectionId}")]
public ActionResult Xyz(int id, int collectionId)

And it's really obvious what's happening without having to change file and look through the route table. I probably wouldn't do it on actions with obvious routes though (like index).

You can read more about attribute routing here.

Answer 2

ASP.NET MVC will produce a controller instance for each request, so you need a bit of help from the framework to assist you with caching - because anything you cache at controller level will only live for a single request's lifetime. As always, Stack Overflow has answers for you :)

// GET: Surveys

// GET: Surveys/Details?name=SomeSurveyName

These comments are somewhat redundant. Unless you've massively changed how routing works in your application, the controller's name and each ActionResult method already says everything this comment says.

---

This part could use a more ...vertical layout. Note that when you're using C# object initializer syntax with a parameterless constructor (i.e. new SurveyModel() { ... }), the parentheses are redundant.

var surveys = surveyMonkey.GetSurveyList()
                          .Select(s => new SurveyModel 
                          {
                              Name = s.Nickname,
                              ResponseCount = s.NumResponses,
                              Url = s.AnalysisUrl 
                          });

Note that this result is deferring the instantiation of SurveyModel objects to the view, which iterates the surveys.

Same here:

var survey = surveyMonkey.GetSurveyList()
                         .Where(s => s.Nickname == name)
                         .Select(s => new SurveyModel
                         { 
                             Name = s.Nickname,
                             ResponseCount = s.NumResponses,
                             Url = s.AnalysisUrl
                         })
                         .First();

The .First() is a little bit confusing here. If the monkey API can only ever return 1 survey for a given name, .SingleOrDefault() would be a much better choice - it would be documenting the fact that a survey has a unique name, and it would return null when no match is found.

Note that .First() will throw an InvalidOperationException - "Sequence contains no element" if the API returns no item for the specified name.

Answer 3

There's some duplication in the SurveysController. Instead of creating a new instance of the SurveyMonkeyApi in each action, create it in the constructor instead.

namespace SurveyMonkeyPlayground.Controllers
{
    public class SurveysController : Controller
    {
        private SurveyMonkeyApi _surveyMonkey;
        public SurveysController(ISurveyMonkeyApiFactory factory)
        {
            _apiFactory = factory.Create();
        }

        // GET: Surveys
        public ActionResult Index()
        {
            var surveys = _surveyMonkey.GetSurveyList()
                            .Select(s => new SurveyModel() { Name = s.Nickname, ResponseCount = s.NumResponses, Url = s.AnalysisUrl });

            return View(surveys);
        }

        // GET: Surveys/Details?name=SomeSurveyName
        public ActionResult Details(string name)
        {
            var survey = _surveyMonkey.GetSurveyList()
                            .Where(s => s.Nickname == name)
                            .Select(s => new SurveyModel() { Name = s.Nickname, ResponseCount = s.NumResponses, Url = s.AnalysisUrl })
                            .First();

            return View(survey);
        }
    }
}

Also, although the factory was a nice idea, you're using an IoC and can do the same job with Ninject by telling it what constructor arguments to use. This means you can completely remove the factory class and interface.

    private static void RegisterServices(IKernel kernel)
    {
        kernel.Bind<SurveyMonkey.SurveyMonkeyApi>()
            .To<SurveyMonkey.SurveyMonkeyApi>()
            .WithConstructorArgument("apiKey", Environment.GetEnvironmentVariable("SM_APIKEY"))
            .WithConstructorArgument("oAuthSecret", Environment.GetEnvironmentVariable("SM_TOKEN"));
    }

Which changes the controller constructor to this.

    private SurveyMonkey.SurveyMonkeyApi _surveyMonkey;
    public SurveysController(SurveyMonkey.SurveyMonkeyApi surveyMonkey)
    {
        _surveyMonkey = surveyMonkey;
    }

Nice and neat and no work has to happen in the ctor.

So, back to the controller's actions. There's more duplication in the way of creating models from the library's Survey objects.

 var surveys = _surveyMonkey.GetSurveyList()
                        .Select(s => new SurveyModel() { Name = s.Nickname, ResponseCount = s.NumResponses, Url = s.AnalysisUrl });

There are two options here.

1. Create a convenience constructor that takes in the external type and maps it to your internal model.

   public class SurveyModel
   {
       public string Name { get; set; }
       public int ResponseCount { get; set; }
       public string Url { get; set; }
   
       public SurveyModel(SurveyMonkey.Survey survey)
       {
           Name = survey.Nickname;
           ResponseCount = survey.NumResponses;
           Url = survey.AnalysisUrl;
       }
   }
   

2. Bind directly to the external type.

   @model SurveyMonkey.Survey
   

The first one makes it much easier to adapt the external type to a slimmed down version. The second one removes this need entirely, but you lose the ability to use the template views that Visual Studio provides. Pick your poison I suppose.