6
\$\begingroup\$

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

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>
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Hmm... I like how the extension method centralized the logic for mapping the data to the model, but one of the reasons I'm looking into asp-MVC is to leverage TDD. This should be okay though (I think). Thank you. Great catch on directly placing the URL btw! I'll definitely read up on routing and that attribute. \$\endgroup\$ – RubberDuck Jul 20 '15 at 9:27
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ You're right about the comments, but as a beginner, they really help me. Spot on about First() though. That was a dirty hack to get it working. Glad you spotted it. I actually need to add Id to the model and query based on it instead. \$\endgroup\$ – RubberDuck Jul 18 '15 at 21:40
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Caching the surveys will significantly change your approach to this. Have you checked out the SO link? \$\endgroup\$ – Mathieu Guindon Jul 19 '15 at 16:52
  • \$\begingroup\$ I have looked at that @Mat'sMug, but haven't put much thought into it yet. I'm thinking I may either ditch that library or fork it though. It has some real issues which are preventing me from moving forward. \$\endgroup\$ – RubberDuck Jul 19 '15 at 16:58

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.