More refactorings from the trenches

A little earlier this week I came across these JavaScript functions while visiting some code in the application I’m working on:

function view_map(location) {

    window.open(‘http://www.google.co.uk/maps?q=‘ + location.value);

    return false;

}

 

function view_directions(form) {

 

    var fromLocation = jQuery(form).find(".from_point").get(0);

    var toLocation = jQuery(form).find(".to_point").get(0);

 

    window.open(‘http://www.google.co.uk/maps?q=from:’ + fromLocation.value + ‘ to:’ + toLocation.value);

    return false;

}

 

These methods are used to open links to Google maps* pages. 

Take note – to consume either of the JavaScript functions, you have to provide specific DOM elements and the form needs to have other fields stashed away the mark-up.

So let’s see how these were being consumed:

 

<a href="#" title="Click to show map" onclick="view_map(jQuery(this).closest(‘.journey’).find(‘.to_point’).get(0));" >To</a>

 

<%–and much further down in the markup–%>

 

<input name="FromLatLng" class="hidden from_point" type="text" value="<%= Model.FromLatLng %>"/>

<input name="ToLatLng" class="hidden to_point" type="text" value="<%= Model.ToLatLng %>"/>

 

Yuck. There’s not a lot going for this code.

Particularly nasty is that code will traverse the DOM looking for particular fields. It then builds up a query string based on the field values, and once clicked, open the link in a new window.

 

Let’s try again.

In this example, the values eventually passed to the JS function,to construct the URI are actually known upfront. So instead, let’s build a small helper extension to construct our Uri from a given location. We’ll hang this helper off of the UrlHelper for convenience:

 

public static string GoogleMap(this UrlHelper urlHelper, string location)

{

    var uri = new UriBuilder("http://www.google.co.uk/maps")

                {

                    Query = string.Format("q={0}", HttpUtility.UrlEncode(location))

                };

 

    return uri.ToString();

}

 

Then, let’s consume it:

 

<a href="<%= Url.GoogleMap(Model.ToLatLng) %>"  target="_blank" title="Click to show map">To</a>

 

This is much less brittle as we no longer traverse the DOM for no reason. We can also remove the unnecessary JavaScript functions, instead using the target attribute of the anchor to provide the same functionality.

 

 

 

*Google provides means to provide maps inside your application and you should use them – using the approach shown above directs users away from your site.

A refactoring from the trenches

A little earlier this week I came across this JavaScript function while visiting some code in the application I’m working on:

 

function filter_form(form) {

    var form = jQuery(form);

    var action = form[0].action;

 

    //get the serialized form data

    var serializedform = form.serialize();

  

    //redirect

    location.href = action + "?" + serializedform;

}

 

The function takes a form, serializes it, and adds its values to the URL, and redirects to it. I found the need to do this is a little odd, so I had a look at the calling code:

 

<form action="<%= Url.Action<SomeController>(x => x.FilterAction()) %>"

method="post" 

class="form-filter">

 

       <%–some input fields here–%>

 

        <a href="#" onclick="var form = jQuery(this).closest(‘.form-filter’); filter_form(form); return false;"

        title="Filter this"

        class="ui-button ui-state-default" 

        id="filter-form"

        rel="filter">Go</a>

 

</form>

 

The exhibited behaviour is that clicking the link (styled as a button) will cause a redirect to the target page with the addition of some query parameters.

Although we can see the developers intent (providing filters as part of the query string), the implementation leaves a lot to be desired.

Let’s see if we can do better.

 

We have a couple of clues here already; the form represents “search filters” and so is idempotent (i.e., causes no side effects).

 

Instead of a JavaScript driven redirect, why not submit the form using HTTP get:

get: With the HTTP "get" method, the form data set is appended to the URI specified by the action attribute (with a question-mark ("?") as separator) and this new URI is sent to the processing agent.

 

<form action="<%= Url.Action<SomeController>(x => x.FilterAction()) %>" method="get">

       <%–some input fields here–%>

<button title="Filter this" type="submit" value="Go">Go</button>

</form>

 

This way we get the same behaviour, minus the unnecessary JavaScript.