Javascript: refactor into framework

Question

I've got a horrible piece of jQuery code in one of my Rails views that listens for an event that looks something like this:

<% user_request_file = params[:file] == document.id.to_s %>
<% if user_request_file %>
    <script type="text/javascript">
        $(document).ready(function() {
            $("#generate_file_btn_for_<%= document.id %>").attr('disabled', 'disabled').attr("href", "#").attr('title', 'Loading file...');
            var doc_modal_<%= document.id %> = setInterval(function() {
                $.getJSON( "<%= file_xhr_document_path(document) %>", function(file) {
                    if (file.file_generated) {
                        $("#loading_for_<%= document.id %>").fadeOut( "slow");
                        $('#document_file_modal_<%= document.id %>').modal('show');
                        $("#file_url_download_link_<%= document.id %>").attr("href", file.file_url);
                        clearInterval(doc_modal_<%= document.id %>);
                    } else if (file.file_error != null) {
                        window.location.href = "<%= file_error_document_path(document) %>";
                    }
                });
            }, 2000);
        });
    </script>
<% end %>

I want to replace it with something more modular and refactor-friendly. The code above listens for a request sent to its server every two seconds, and if it has been sent the view responds with information from that request.

Flight has been recommended to me as a suitable replacement framework but I'm really not sure how to begin to refactor it. Any advice or even recommendations other frameworks would be great.

The first step is to pull the JavaScript out of the template/text-replacement context - this applies equally as much if moving to Flight or KO or whatever else - so that it can be treated as it's own/standalone module. Although to a different environment, I've provided an example here. The point is the JavaScript is isolated behind a relevant API and values are supplied. — user2864740
– user2864740, Commented Sep 16, 2014 at 23:52

maček · Accepted Answer · 2014-09-17 00:28:50Z

Here's one way you could make it a little better

(function($) {

  // store the information we care about once
  var doc = {
    file:     '<%= j params[:file] %>'
    id:       '<%= j document.id %>',
    path: {
      xhr:    '<%= j file_xhr_document_path(document) %>',
      error:  '<%= j file_error_document_path(document) %>'
    }
  };

  // stop immediately if file !== doc.id
  if (doc.file !== doc.id) return;

  function getJSON() {
    var xhr = $.getJSON(doc.path.xhr);
    xhr.done(onSuccess);
    xhr.fail(onError);
  }

  function onSuccess(file) {
    if (!file.file_generated) return onError();
    $("#loading_for_" + doc.id).fadeOut("slow");
    $("#document_file_modal_" + doc.id).modal("show");
    $("#file_url_download_link_" + doc.id).attr("href", file.file_url);
    setTimeout(getJSON, 2000);
  }

  function onError() {
    window.location.href = doc.path.error;
  }

  function init(event) {
    $("#generate_file_btn_for_" + doc.id)
      .prop("disabled", true)
      .attr("title", "Loading file...");
    getJSON();
  }

  if (doc.id) $(document.ready(init);
})(jQuery);

Explanation

The first thing you'll notice is how much flatter the code is. At it's deepest, this code has 2 levels of indentation. The existing code you have is 4. Maybe it's a personal preference, but I find flatter code much easier to read.

This bit of code stores all of the pertinent vars in a little object for us to reuse in the rest of the script. This cleans up the JavaScript quite a bit as it removes tons of erb calls.

var doc = {
  id:  <%= j document.id %>,
  path: {
    xhr: <%= j file_xhr_document_path(document) %>,
    error: <%= j file_error_document_path(document) %>
  }
};

From here, other improvements would be to remove $(...) calls from within the functions that do work. A better approach would be to create a function wrapper around the getJSON bits and pass in the elements you'll be acting upon. It's always better to pass dependencies into the your functions that do work rather than statically couple them within the function bodies.

This is just a start

This is by no means the cleanest the code could be. I have no idea what other scripts/tools you have available, but hopefully this gives you an idea of how to start making some improvements.

That's a much better solution (I didn't read it properly before).

Collectives™ on Stack Overflow

Javascript: refactor into framework

1 Answer 1

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related