4
\$\begingroup\$

This js was written strictly for a visualization of a graph redesign included in a stats report within our game systems. It all works properly, but I know there has to be a much neater way to organize this. I have some ideas, but I don't quite know how to approach it. Any guidance would be greatly appreciated.

    var barContainer = document.getElementById('bar-graph');
    var barCount = 15;

    for (index = 0; index < barCount; ++index) {
        var liBar = document.createElement('li');
        liBar.innerHTML = '<div class="bar"></div>';
        barContainer.appendChild(liBar);
    }

    //this is for visualization. Sets height of each bar, then adds a margin to the top of each based on height, in order to keep each bar flushed with the bottom of the graph.
    var list = document.getElementsByClassName("bar");
    var percentGroup = new Array();

    for (index = 0; index < list.length; ++index) {
        var bar = list[index];
        var originalHeight = $(bar).height();
        $(bar).css( "height", originalHeight - (index * 10) );
        var barHeight = $(bar).height();
        var percentage = barHeight / originalHeight;
        percentGroup.push(percentage);
        var containerHeight = $('.graph').height();
        var marginTop = containerHeight - barHeight;
        $(bar).css('margin-top', marginTop);
        var graph = $('#bar-graph');
        var graphWidth = graph.width();
        var barWidth = $(bar).width();
        var marginRight = ( graphWidth - (barWidth * barCount) ) / ( barCount - 1 );
        $(".graph ul li").css('margin-right', marginRight);
        $(".graph ul li:nth-last-child(2)").css('margin-right', (marginRight - 1) );
    }



    //this function takes the decimal value percentages generated above, turns them into integers, and multiplies them by 100. It also creates the containers that house the percentages
    var val = document.getElementById('valContainer');

    for (index = 0; index < percentGroup.length; ++index) {
        var bar = percentGroup[index];
        bar = Math.round(bar * 100);
        var li = document.createElement('li');
        li.innerHTML = bar + '%';
        val.appendChild(li);
        $(val).css('width', (graphWidth + 4) );
        $(li).css('margin-right', marginRight);
    }
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

I have a hard time figuring out what the code is doing, especially since you didn't include the corresponding HTML, but I can say that there is too much code for the task. Ignoring the valContainer and percentGroup display, the code to render the plot should look a lot simpler, like this:

function barGraph($barContainer, data) {
    var dataMax = Math.max.apply(null, data);      
    for (var i = 0; i < data.length; ++i) {
        $barContainer.append(
            // Use 90 instead of 100 for the height multiplier to leave
            // some space at the top for aesthetics.
            $('<li></li>').css('height', data[i] * 90 / dataMax + '%')
                          .css('left',   i * 100 / data.length + '%')
                          .css('width',  100 / data.length + '%')
        );
    }
}

barGraph($('#bar-graph'), [3, 1, 4, 1, 5, 9, 2, 6, 5]);
.graph ul {
  background-color: lightgray;
  list-style: none;
  position: relative;
  height: 150px;
}
.graph ul li {
  display: inline-block;
  position: absolute;
  bottom: 0;
  background-color: red;
  box-shadow: inset 1px 0 lightgray;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<div class="graph">
  <ul id="bar-graph"></ul>
</div>

Specifically,

  • The code should live in a function. It should be clear what the parameters to the function are.
  • Avoid magic numbers. What is 15? Should it just be list.length?
  • Loop counter variables are typically named i. Make sure it is localized with var.
  • If you're using jQuery, avoid raw DOM access, which is verbose and confusing.
  • Extract loop invariants, such as $('.graph').height(), out of the loop. Better yet, use percentages as your CSS units, which both simplifies the calculation and allows automatic resizing if the container is resized.
  • Use CSS to your advantage. In this case, absolute positioning would avoid most of the calculation headaches.
\$\endgroup\$
2
  • \$\begingroup\$ Taking this as the answer, as it encompasses the wonderful information offered in @Oli 's answer and offers an even simpler solution. Thank you, 200_success. This was extremely helpful! \$\endgroup\$ Jun 3, 2016 at 14:01
  • \$\begingroup\$ Also, the 15 was a number chosen arbitrarily for presentation purposes, and will later become list.length whenever I'm able to use real data. \$\endgroup\$ Jun 3, 2016 at 14:17
2
\$\begingroup\$

Too many temporary variables

Try to reduce the number of local temporary variables as they make the code muddy and unnecessary complicated and difficult to change.

Example, these four lines could probably be reduced to a single line:

var barHeight = $(bar).height();
...
var containerHeight = $('.graph').height();
var marginTop = containerHeight - barHeight;
$(bar).css('margin-top', marginTop);

Could be:

$(bar).css('margin-top', $('.graph').height() - $(bar).height());

Delegate tasks to functions

It is not a bad thing to sometimes delegate to functions for clearity at the exepence of minor performance hit:

var graph = $('#bar-graph');
var graphWidth = graph.width();
var barWidth = $(bar).width();
var marginRight = ( graphWidth - (barWidth * barCount) ) / ( barCount - 1 );
$(".graph ul li").css('margin-right', marginRight);
$(".graph ul li:nth-last-child(2)").css('margin-right', (marginRight - 1) );

Could become:

$(".graph ul li").css('margin-right', getRightMargin(bar, barCount));
$(".graph ul li:nth-last-child(2)").css('margin-right', (getRightMargin(bar, barCount) - 1) );
...
function getRightMargin(bar, barCount) {
    var graphWidth = $('#bar-graph').width();
    var barWidth = $(bar).width();
    return ( graphWidth - (barWidth * barCount) ) / ( barCount - 1 );
}

This way the margin calculation will be done twice, but the performance implication in this case is negligible, the code however will be less noisy and more approachable.

\$\endgroup\$
1
  • \$\begingroup\$ Welcome, and I agree, @200_success had a great answer. \$\endgroup\$
    – Oli
    Jun 3, 2016 at 17:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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