0
\$\begingroup\$

I've written a small bit of JavaScript which parses some HTML and generate a table of contents based on the various heading tags. The table of contents needs to be nested (E.g. H2 tags appear under H1).

My code is currently working well but I'm not sure the my method of storing all the headings is the best (it's been awhile since I've had to do much JS). The headings are being stored in a series of nested objects. The objects are made up of two properties (#title and #id) to store the heading title and anchor link. It can then contain incrementally numbered sub-heading objects.

A large portion of my code seems to be dealing with finding the last item in the object to append the next heading to. I don't know if there would be an easier way of doing this with nested arrays.

The script runs the parseDOM function passing it the DIV containing the report. Once that's complete the resulting object is passed to a series of functions which turns it into a series of ordered list elements.

I've stripped out the parts for generating the table of contents HTML, I can post it if you want it though.

var heading_selectors = [
 'H1',
 'H2',
 'H3',
 'H4',
 'H5',
 'H6',
];
function parseDOM(report) {
  // Generate selector from heading selectors.
  var selector = heading_selectors.join(',');
  var nav_hierarchy = {};
  var next_heading_id = 0;
  $(selector, report).each(function(index, heading_element) {
    var heading = $(heading_element);
    // Determine heading level.
    var tag_name = heading.get(0).nodeName;
    var level = heading_selectors.indexOf(tag_name);
    var title = heading.text();
    var id = next_heading_id;
    var id_name = id + '-' + level + '-' + title.replace(/\s+/g, '-').toLowerCase();
    heading.attr('id', id_name);

    // Create object to hold heading.
    var new_heading = {
      '#title': title,
      '#id': id_name,
    };

    var parent_item = lastHeadingAtLevel(nav_hierarchy, level);
    parent_item[id] = new_heading;

    // Increment heading id.
    next_heading_id++;
  });

  return nav_hierarchy;
}

/**
 * Returns the last heading at the specified level.
 *
 * @param object heading Heading to retrieve the last element of.
 * @param int level What level of heading to retrieve from.
 *
 * @return object Last heading at specified level.
 */
function lastHeadingAtLevel(heading, level) {
  if (level !== 0) {
    // Recurse through lists until you hit the required level.
    var last_item = getLastHeading(heading);
    last_item = lastHeadingAtLevel(last_item, level-1);
    return last_item;
  }
  // No recursion required.
  return heading;
}

/**
 * Return the last heading in the object (excludes properties beginning with #).
 *
 * @param object heading Heading to retrieve last heading of.
 *
 * @return object Last Heading of object.
 */
function getLastHeading(heading) {
  // Get the keys of the current heading.
  var keys = Object.keys(heading);
  // Default last_element to lowest value
  var last_element = -1;
  for (var i = 0; i < keys.length; i++) {
    if (keys[i].startsWith('#')) {
      continue;
    }
    if (parseInt(keys[i], 10) > last_element) {
      last_element = parseInt(keys[i], 10);
    }
  }

  // Return the last element added.
  return heading[last_element];
}
\$\endgroup\$
2
\$\begingroup\$

First, your indentation is half-indentation. It should be four spaces instead of two. Try putting your code into JSFiddle's TidyUp function to see how to better format.


You can improve the following block by swapping the logic and not initialising last_item:

You also don't need a === check, == will do for integers.

function lastHeadingAtLevel(heading, level) {
  if (level !== 0) {
    // Recurse through lists until you hit the required level.
    var last_item = getLastHeading(heading);
    last_item = lastHeadingAtLevel(last_item, level-1);
    return last_item;
  }
  // No recursion required.
  return heading;
}

by applying those changes, you get the following:

function lastHeadingAtLevel(heading, level) {
    if (level == 0) {
        // No recursion required.
        return heading;
    }
    // Recurse through lists until you hit the required level.
    var last_item = getLastHeading(heading);
    return lastHeadingAtLevel(last_item, level - 1);
}

Instead of string concatenation (can be slow), you should use an array join instead (faster):

var id_name = id + '-' + level + '-' + title.replace(/\s+/g, '-').toLowerCase();

into:

var id_name = [id, level, title.replace(/\s+/g, '-').toLowerCase()].join('-');

In the following block, keys is an array, so instead of a for loop, use a forEach loop:

  for (var i = 0; i < keys.length; i++) {
    if (keys[i].startsWith('#')) {
      continue;
    }
    if (parseInt(keys[i], 10) > last_element) {
      last_element = parseInt(keys[i], 10);
    }
  }

which could transform it into the following:

var lastElement = -1;
Object.keys(heading).forEach(function(key){
    if (key.startsWith('#')) {
        continue;
    }
    if (parseInt(key, 10) > lastElement){
        lastElement = parseInt(key, 10);
    }
});

However, in fact, you don't even need to do that.

Using a polyfill, you can access the last item:

if (!Object.prototype.last){
    Object.prototype.last = function(){
        return this[Object.keys(this)[Object.keys(this).length - 1]];
    };
};

Now just access it like: {'a': 1, 'b': 2, 'c': '3'}.last();

\$\endgroup\$

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.