6
\$\begingroup\$

I'm working on reworking our website from a WYSIWYG editor to MVC. I'm all right with server side code, but complete rubbish when it comes to client side Javascript, so I'd appreciate any/all feedback.

The goal

Embed this rss feed from our blog into a page on our main website.

So that it looks like this:

enter image description here

Design choices/rationale

I had originally written this code to go get the RSS feed in the News controller, and bind it to the News view. This caused unacceptable load times for the page. Obviously, the page couldn't render anything until it had gone across the network to a page, that may or may not be available, and retrieve the RSS feed. It took upwards of 5-6 seconds for anything at all to render my browser.

So, I decided to try my hand at retrieving this and loading it client side after allowing the "container" of the page to load. I extracted a BlogFeed partial view that is retrieved and inserted by an Ajax call after the page is ready. This allows initial rendering to happen quickly, only later filling in the content that takes time to retrieve.

I considered either creating a separate BlogFeedService class, a separate News controller, or both, but it felt like over engineering a simple site that could almost be served up as static HTML. If either the Home controller, or News code grows any more, I will likely extract a few classes.

I used MVC so that I could extend the site with more dynamic behavior later. I intend on adding a preview of some of our project's functionality through a webapp section of this site.

Questions

Is this a good approach? I'll admit that I basically copy/pasta'd the jquery code, then monkey patched it with a Url.Action to make sure I wasn't hardcoding Urls into it.

Did I do the async/await stuff correctly? I'm never quite sure I've gotten that correct either. Secondarily, was using async await here overkill? It felt right, because reaching out over the network is potentially blocking, but there's not much for the rest of the code to do while it waits.

Again, any/all feedback is appreciated. I've only been doing web development for a few months.

Note: The Twitter code was generated by Twitter and simply embedded into the view. It's not mine.

News.cshtml

@{
    ViewBag.Title = "News";
}

<section class="row lead">
    <h2>What's up duck?</h2>

    <p>
        When we have an announcement to make, we publish on <a href="https://rubberduckvba.wordpress.com/" target="_blank"><strong>Rubberduck News</strong></a>.
        If you're following us on Twitter or WordPress, you'll be notified right away.
        Here's what you might have been missing lately...
    </p>
</section>
<div class="row">
    <h3>Rubberduck News</h3>
    <section id="blogFeed" class="col-md-9">
        <!--We'll load this client side to speed up inital page load-->
    </section>

    @section scripts{
    <script>
        $(document).ready(function() {
            //Url will look something like this '/home/BlogFeed/'
           $.get(@String.Concat(Url.Action("BlogFeed"), "/"), function(data) {
                $('#blogFeed').html(data);
            });
        });
    </script>
    }

    <div id="twitterFeed" class="col-md-3">
        <a class="twitter-timeline" href="https://twitter.com/rubberduckvba" data-widget-id="689036212724723712" height="1200">Tweets by @@rubberduckvba</a>
        <script>
            !function(d, s, id) {
                var js, fjs = d.getElementsByTagName(s)[0], p = /^http:/.test(d.location) ? 'http' : 'https';
                if (!d.getElementById(id)) {
                    js = d.createElement(s);
                    js.id = id;
                    js.src = p + "://platform.twitter.com/widgets.js";
                    fjs.parentNode.insertBefore(js, fjs);
                }
            }(document, "script", "twitter-wjs");
        </script>
    </div>
</div>

BlogFeed.cshtml

@model IEnumerable<System.ServiceModel.Syndication.SyndicationItem>

@if (Model == null)
{
    <p class="alert alert-danger">Aww snap! We couldn't retrieve the RSS Feed!</p>
}
else
{
    foreach (var post in Model)
    {
        <div class="row feedSnippet col-md-12">
            <h4>
                <!--Id is the permalink-->
                <a href="@post.Id" target="_blank">@post.Title.Text</a> <small>@post.PublishDate.Date.ToLongDateString()</small>
            </h4>
            <p>@Html.Raw(post.Summary.Text)</p>
        </div>
    }
}

HomeController.cs

using System;
using System.Collections.Generic;
using System.Net.Http;
using System.ServiceModel.Syndication;
using System.Threading.Tasks;
using System.Web.Mvc;
using System.Xml;

namespace RubberduckWeb.Controllers
{
    public class HomeController : Controller
    {
        public ActionResult Index()
        {
            return View();
        }

        public ActionResult Features()
        {
            return View();
        }

        public ActionResult About()
        {
            return View();
        }

        public ActionResult Contact()
        {
            return View();
        }

        public ActionResult News()
        {
            return View();
        }

        public async Task<PartialViewResult> BlogFeed()
        {
            return PartialView(await GetBlogFeedItemsAsync());
        }

        private async Task<IEnumerable<SyndicationItem>> GetBlogFeedItemsAsync()
        {
            const string rssUri = "https://rubberduckvba.wordpress.com/feed/";

            using (var httpClient = new HttpClient())
            {
                var response = await httpClient.GetAsync(new Uri(rssUri));

                if (!response.IsSuccessStatusCode)
                {
                    return null;
                }

                using (var reader = XmlReader.Create(await response.Content.ReadAsStreamAsync()))
                {
                    var feed = SyndicationFeed.Load(reader);
                    return feed?.Items;
                }
            }
        }
    }
}
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

A couple of really quick comments:

jQuery provides a load function which simplifies your code. There's also a shorthand for $(document).ready(function() {}); which is simply $(function() {});

Putting that together:

$(function() {
    $('#blogFeed').load('@Url.Action("BlogFeed")');
});

That's a bit nicer!

I'd prefer Razor server side commenting. E.g.

<!--Id is the permalink-->

becomes:

@* Id is the permalink *@

That way the comment is never sent to the client browser.

Your comment about async and await:

It felt right, because reaching out over the network is potentially blocking, but there's not much for the rest of the code to do while it waits.

By not blocking, the framework can respond to more client requests! It's definitely the right thing to be doing.

Update: String.Concat explanation:

$.get(@String.Concat(Url.Action("BlogFeed"), "/"),

This code will render the following js:

$.get(/BlogFeed/, 

Which is a regex literal but being treated like a string. However, this only works because it's on the default controller and so doesn't have any slashes. If it had been on it's own controller e.g. 'News/BlogsFeed' it wouldn't have worked.

\$\endgroup\$
6
  • \$\begingroup\$ Oh! Good point! I hadn't thought about the server responding to other incoming requests! Cool! Thanks for the jQuery pointers too! Load looks much nicer than what I've done. Quick question, does load(url) require the trailing slash too? get seemed to need it, hence the String.Concat garbage. \$\endgroup\$
    – RubberDuck
    Commented Jan 19, 2016 at 15:18
  • \$\begingroup\$ @RubberDuck - I don't think so. What happened when you omitted the final slash? As you aren't using quotes around the value I think that it's probably being treated as a regex literal when you have both slashes. \$\endgroup\$
    – RobH
    Commented Jan 19, 2016 at 15:23
  • \$\begingroup\$ @RubberDuck I've added some extra comments about the String.Concat \$\endgroup\$
    – RobH
    Commented Jan 19, 2016 at 15:29
  • 1
    \$\begingroup\$ Oh wow. I totally misunderstood what I was doing there. Thanks @RobH! This is likely to get a checky, but I'll wait to see if any other answers come in first. \$\endgroup\$
    – RubberDuck
    Commented Jan 19, 2016 at 15:43
  • 2
    \$\begingroup\$ Remember to be VERY PARANOID about using .load(url). I repeat: VERY PARANOID! If your url comes empty or has a falsy value, it may be interpreted as calling .load(), which will trigger the onload event. It sounds silly, I know, but watch for that. You should just use $.ajax(), which will let you set other options that you may need in the future. But for now, $.get seems enough. \$\endgroup\$ Commented Jan 19, 2016 at 16:30

Not the answer you're looking for? Browse other questions tagged or ask your own question.