3
\$\begingroup\$

I have written a jQuery script to create keyboard shortcuts to highlight elements of a class (given to me) and toggle through them. I have created the HTML file myself, so ids and class names are all by me.

The class names are debug, stacktrace, traceback and error. Initially first element of every class has the id current_debug, current_stacktrace, current_traceback, current_error.

Here's my code to create keyboard shortcuts:

$(document).keypress(function(e){
    e=e||window.event;
    var keyCode = e.keyCode || e.which;
    if(keyCode==103){//g
        reset();
        // $('.debug').addClass('highlight');
        $('.debug').addClass('highlight').addClass('yellow');
        $('html, body').animate({
            scrollTop: $("#current_debug").offset().top
        }, 500);
    }
    else if(keyCode==105){//i
        reset();
        $('.error').addClass('highlight').addClass('red');
        $('html, body').animate({
            scrollTop: $("#current_error").offset().top
        }, 500);
    }
    else if(keyCode==106){//j
        reset();
        $('.stacktrace').addClass('highlight').addClass('violet');
        $('html, body').animate({
            scrollTop: $("#current_stacktrace").offset().top
        }, 500);
    }
    else if(keyCode==107){//k
        reset();
        $('.traceback').addClass('highlight').addClass('purple');
        $('html, body').animate({
            scrollTop: $("#current_traceback").offset().top
        }, 500);
    }
    else if(keyCode==108){//l
        toggledown();
    }
});

Here's my reset function:

function reset(){
    $('.debug').removeClass().addClass('debug');
    $('.error').removeClass().addClass('error');
    $('.traceback').removeClass().addClass('traceback');
    $('.stacktrace').removeClass().addClass('stacktrace');
}

Here's my toggledown function:

function toggledown(){
    var classname=$('.highlight').attr('class').split(" ")[0];
    var id= "current_"+classname;
    classitem=$('.'+classname);
    var item=$('#'+id);
    var index = classitem.index(item);
    classitem.eq(index-1).removeClass('selected');
    item.addClass('selected');
    if(!(item.isVisible())){
        $('html, body').animate({
            scrollTop: item.offset().top
        }, 500);
    }
    item.removeAttr('id');
    classitem.eq(index+1).attr('id',id);
}

And here's my isVisible function:

$.fn.isVisible = function() {
    var elementTop = $(this).offset().top;
    var elementBottom = elementTop + $(this).outerHeight();

    var viewportTop = $(window).scrollTop();
    var viewportBottom = viewportTop + $(window).height();

    return elementBottom > viewportTop && elementTop < viewportBottom;
};

The code works just as I want it to work. But for large HTML pages, there is a noticeable delay in the execution of code - especially for toggle down. It almost takes two or three seconds to toggle from one element to another.

This is the first time I am writing anything really useful in jQuery. Any tips/advice on how I can make this code better and run faster is very much appreciated.

jsFiddle

Please remember that the problem is only for very large HTML files.

I have since read other sources, and realized that my not caching all the variables was a bad practice. So I cached them and this is my code now:

var placeholder;
var curclass;
var debug=$('.debug');
var error = $('.error');
var traceback=$('.traceback');
var stacktrace=$('.stacktrace');

This caches all the variables I repeatedly use. Here are the modified functions accordingly:

$.fn.isVisible = function() {
    var elementTop = $(this).offset().top;
    var elementBottom = elementTop + $(this).outerHeight();

    var viewportTop = $(window).scrollTop();
    var viewportBottom = viewportTop + $(window).height();

    return elementBottom > viewportTop && elementTop < viewportBottom;
};
function toggledown(){
    var id="current_"+placeholder;
    console.log(id);
    console.log(placeholder);
    var item=$('#'+id);
    var index=curclass.index(item);
    curclass.eq(index-1).removeClass('selected');
    item.addClass('selected');
    if(!(item.isVisible())){
        $('html,body').animate({
            scrollTop:item.offset().top
        },0);
    }
    item.removeAttr('id');
    curclass.eq(index+1).attr('id',id);
}
function reset(){
    placeholder='';
    debug.removeClass().addClass('debug');
    error.removeClass().addClass('error');
    traceback.removeClass().addClass('traceback');
    stacktrace.removeClass().addClass('stacktrace');
}
$(document).keypress(function(e){
    e=e||window.event;
    var keyCode = e.keyCode || e.which;
    if(keyCode==103){//g
        reset();
        curclass=debug;
        placeholder='debug';
        debug.addClass('highlight').addClass('yellow');
        $('html, body').animate({
            scrollTop: $("#current_debug").offset().top
        }, 500);
    }
    else if(keyCode==105){//i
        reset();
        curclass=error;
        placeholder='error';
        error.addClass('highlight').addClass('red');
        $('html, body').animate({
            scrollTop: $("#current_error").offset().top
        }, 500);
    }
    else if(keyCode==106){//j
        reset();
        curclass=stacktrace;
        placeholder='stacktrace';
        stacktrace.addClass('highlight').addClass('violet');
        $('html, body').animate({
            scrollTop: $("#current_stacktrace").offset().top
        }, 500);
    }
    else if(keyCode==107){//k
        reset();
        curclass=traceback;
        placeholder='traceback';
        traceback.addClass('highlight').addClass('purple');
        $('html, body').animate({
            scrollTop: $("#current_traceback").offset().top
        }, 500);
    }
    else if(keyCode==108){//l
        toggledown();
    }
});

Sadly, the performance improvement wasn't really high. Kindly let me know how I can improve this code further.

\$\endgroup\$
  • \$\begingroup\$ Good job on your first Code Review question, and thank you for declaring your cross-post. You could improve this question further by including the corresponding HTML code as well — ideally by making a live demo. (Press Ctrl-M in the question editor.) \$\endgroup\$ – 200_success Jun 14 '17 at 3:53
  • \$\begingroup\$ Thank you! I added a working js fiddle, hope that is sufficient. \$\endgroup\$ – Vamshi Jun 14 '17 at 4:54
1
\$\begingroup\$

You aren't using CSS effectively, and as a result, you are doing too much micromanagement using JavaScript. The CSS selectors can have some "intelligence" built in, so that you can take advantage of the rule engine that is built into the browser. You shouldn't be enabling and disabling highlighting on each <p> element. Rather, consider the current message type (whether "debug", "error", "traceback", or "stacktrace") to be global state, and the modes are mutually exclusive. On one parent element, just specify which message type you are interested in. Then, CSS can magically enable highlighting on the appropriate child elements. (For another demonstration of this powerful technique, observe how this iOS Notes app replica effortlessly switches between list mode and editing mode.)

Furthermore, your HTML classes and CSS selectors should be specified semantically. Your HTML and JavaScript code should talk about "traceback" rather than "violet". Having a class named "violet" that applies violet as the background color defeats the purpose of using stylesheets, which is to free your HTML and JavaScript code of artistic concerns.

I would consider it bad practice to mutate the id attribute of a DOM element. An id is supposed to uniquely identify an element, so renaming would just cause confusion. (Also, strictly speaking, you also have to ensure that no other element on the page could ever have a conflicting name.)

Minimize the code in the event handler, which should be primarily concerned about dispatching. This is especially true when several of the cases have nearly identical code. For handling multiple keycodes, it is slightly more efficient to use a switch rather than chained if-else statements.

I think that toggledown() is a poorly named function. "Toggle" implies switching something back and forth between two states, but that's not what it does. This function advances to the next element for the current message type, so I'd call it advance(). A JavaScript error occurs when advancing beyond the last element; I've decided to wrap around to the first element in that case.

Note that some of the HTML elements in your JSFiddle are mislabled as debug lines, when they are actually errors, stacktraces, or tracebacks.

var $log = $('.log');

$.fn.isVisible = function() {  // Unmodified
    var elementTop = $(this).offset().top;
    var elementBottom = elementTop + $(this).outerHeight();

    var viewportTop = $(window).scrollTop();
    var viewportBottom = viewportTop + $(window).height();

    return elementBottom > viewportTop && elementTop < viewportBottom;
};

function getMessageType() {
    return $log.attr('data-message-type');
}

function setMessageType(msgType) {
    $log.attr('data-message-type', msgType);
    $('html, body').animate({
        scrollTop: $log.children('p.current.' + msgType).offset().top
    }, 500);
}

function advance() {
    var msgType = getMessageType();
    if (!msgType) return;

    var $msgs = $log.children('p.' + msgType);
    var $current = $msgs.filter('.current');
    var $next = $current.nextAll('p.' + msgType).first();
    if (! $next.length) {
        $next = $msgs.first();  // Last message reached; wrap around
    }
    $current.removeClass('current');
    $next.addClass('current');
    if (! $next.isVisible()) {
        $('html,body').animate({
            scrollTop: $next.offset().top
        },0);
    }
}

$(document).keypress(function(e) {
    e = e || window.event;
    switch (e.keyCode || e.which) {
      case 103:  // g
        setMessageType("debug");
        break;
      case 105:  // i
        setMessageType("error");
        break;
      case 106:  // j
        setMessageType("stacktrace");
        break;
      case 107:  // k
        setMessageType("traceback");
        break;
      case 108:  // l
        advance();
    }
});
div.log > p {
    border-width: 0;
}

div.log[data-message-type=debug] > p.debug {
    background-color: yellow;
    border-width: 3px;
}

div.log[data-message-type=error] > p.error {
    background-color: red;
    border-width: 3px;
}

div.log[data-message-type=traceback] > p.traceback {
    background-color: violet;
    border-width: 3px;
}

div.log[data-message-type=stacktrace] > p.stacktrace {
    background-color: purple;
    border-width: 3px;
}

p.current {
    border-style: dotted;
}
<script type="text/javascript" src="//code.jquery.com/jquery-2.2.4.js"></script>

<div class="log">
  <p class="debug current">This is a debug line</p>
  <p class="error current"> This is an error line </p>
  <p class="stacktrace current"> This is a stacktrace line </p>
  <p class="traceback current"> This is a traceback line</p>
  <p class="debug"> This is another debug line </p>
  <p class="debug"> This is another debug line </p>
  <p class="stacktrace"> This is another stacktrace line </p>
  <p class="traceback"> This is another traceback line </p>
</div>

\$\endgroup\$
0
\$\begingroup\$

I think some good refactoring will go a long way. Here is an alternate fiddle, which covers more than just the performance issues. Hopefully it helps with the performance issues (and there are some changes specifically for performance), but it is hard to say without an example that is actually very slow.

https://jsfiddle.net/sna3LL57/3/

The biggest changes were to refactor the way you used classed, ditch your ids, add a highlight function and refactor your reset function. Here is the relevant code for highlight and reset:

function highlight( type ){
    $('.'+type).addClass('highlight');
    $('html, body').animate({
        scrollTop: $('.'+type).filter( '.current' ).offset().top
    }, 500);
}
function reset(){
    $('.highlight').removeClass( 'highlight' );
    $('.selected').removeClass( 'selected' );
}

Breaking down my changes generally:

1. Use class names that denote meaning, not presentation

You use class names like 'yellow' and 'purple' and add/remove those in javascript. However, the class names you work with in javascript should represent what you are doing in javascript. The exact colors don't matter: that is a styling decisions that is left up to styling (i.e. css). What you care about is highlighting a particular set of entries, so you should be adding/remove the 'highlight' class, not color specific classes. As you can see I then adjusted the color by specifying joint-class rules in the CSS (i.e. if this has a class of highlight and a class of error, then set the background color to red). Keeping class names specific to the purpose (instead of the presentation) does a better job of separation of concerns, and also makes the code much cleaner and easier to read (as you will see).

2. Don't make unnecessary changes to the document

Your reset implementation edits everything, when it could be much more targeted. You remove all classes from every element and then add back on their specific classes. The new version just removes the highlight class from the highlighted elements, and also removes the selected class from the selected elements. It is very targeted, only changing the elements that need to change.

3. Select on class, not id

I ditched the ids and went with a class that gets added to the current element. This could actually still work with an id, but having four different ids just complicates your code. There will only ever be one current element so there is no need to have a separate id for each "kind" of current. The toggledown function has been adjusted so that if no current element is found, then it just makes the first one the current. Looking at your code I'm not sure exactly why toggleDown in particular would be slow: you might have to do some profiling if my changes don't help.

\$\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.