4
\$\begingroup\$

I am working on a UserScript that displays total flag counts and other information on the Flag Summary page.

Example:

flag history with totals

Everything is working correctly but this bit of code seems rather repetitive and I'm sure it could be improved upon.

The following code pulls the total count of each flag type from the flag summary page.

Is there anything I can do to improve or shorten this code?

var helpful = 0,
    declined = 0,
    disputed = 0,
    aged = 0,
    total = 0;

$("td > a:contains('helpful')").parent().prev().each(function () {
    helpful += parseInt($(this).text().replace(",",""));
});

$("td > a:contains('declined')").parent().prev().each(function () {
    declined += parseInt($(this).text().replace(",",""));
});

$("td > a:contains('disputed')").parent().prev().each(function () {
    disputed += parseInt($(this).text().replace(",",""));
});

$("td > a:contains('aged')").parent().prev().each(function () {
    aged += parseInt($(this).text().replace(",",""));
});

$("td > a:contains('flags')").parent().prev().each(function () {
    total += parseInt($(this).text().replace(",",""));
});

Here is an example of what the markup looks like:

<table id="flag-stat-info-table">
    <tbody>
        <tr class="group-name">
            <td class="col1">441</td>
            <td class="col2"><a href="?group=1">post flags</a></td>
        </tr>
        <tr>
            <td class="col1">41</td>
            <td class="col2"><a href="?group=1&amp;status=1">waiting for review</a></td>
        </tr>
        <tr>
            <td class="col1">310</td>
            <td class="col2"><a href="?group=1&amp;status=2">helpful</a></td>
        </tr>
        <tr>
            <td class="col1">5</td>
            <td class="col2"><a href="?group=1&amp;status=3">declined</a></td>
        </tr>
        <tr>
            <td class="col1">36</td>
            <td class="col2"><a href="?group=1&amp;status=4">disputed</a></td>
        </tr>
        <tr>
            <td class="col1">49</td>
            <td class="col2"><a href="?group=1&amp;status=5">aged away</a></td>
        </tr>
        <tr class="group-name">
            <td class="col1">412</td>
            <td class="col2"><a href="?group=4">comment flags</a></td>
        </tr>
        <tr>
            <td class="col1">6</td>
            <td class="col2"><a href="?group=4&amp;status=1">waiting for review</a></td>
        </tr>
        <tr>
            <td class="col1">406</td>
            <td class="col2"><a href="?group=4&amp;status=2">helpful</a></td>
        </tr>

        <tr class="group-name">
            <td class="col1">5</td>
            <td class="col2"><a href="?group=2">spam flags</a></td>
        </tr>
        <tr>
            <td class="col1">5</td>
            <td class="col2"><a href="?group=2&amp;status=2">helpful</a></td>
        </tr>
        <tr>
            <td colspan="2">&nbsp;</td>
        </tr>
        <tr>
            <td></td>
            <td class="disabled-link">Clear all filters</td>
        </tr>
        <tr>
            <td colspan="2">&nbsp;</td>
        </tr>
        <tr>
            <td colspan="2">Filter your flag history by selecting a category above.</td>
        </tr>
    </tbody>
</table>

Note: I've only included the relevant part as I didn't want to add a bunch of noise to this post.

\$\endgroup\$
5
+50
\$\begingroup\$

I don't particularly have an issue with your HTML, however your JavaScript could be improved.

Currently, you have a lot of duplicate logic. You should move it to a function that each case can call.

Like the following:

function flagCount(selector){
    var count = 0;
    $(selector).parent().prev().each(function () {
        count += parseInt($(this).text().replace(",", ""));
    });
    return count;
}
  • If I pass 0 into parseInt without the second parameter, it will return a hex number...

Using Number is better, and more readable.

Leaving you totally with:

function flagCount(selector){
    var count = 0;
    $(selector).parent().prev().each(function () {
        count += Number($(this).text().replace(",", ""));
    });
    return count;
}
var helpful = flagCount("td > a:contains('helpful')");
var declined = flagCount("td > a:contains('declined')");
var disputed = flagCount("td > a:contains('disputed')");
var aged = flagCount("td > a:contains('aged')");
var total = flagCount("td > a:contains('flags')");

Note, you could string concatenate the selector inside the function, but that limits the use case that you select another DOM element.

\$\endgroup\$
1
\$\begingroup\$

First, instead of discrete vars, use object properties :

var types = {
    'helpful': 0,
    'declined': 0,
    'disputed': 0,
    'aged': 0,
    'flags': 0 // note: flags not total
};

This is a good thing to do anyway as it puts just one member into the namespace.

Then, Object.keys() will return an array of keys, which can be looped through :

Object.keys(types).forEach(function(key) {
    $("td > a:contains('" + key + "')").parent().prev().each(function () {
        types[key] += parseInt($(this).text().replace(',',''));
    });
});

Then (if it's important) post-process types to change .flags to .total.

types.total = types.flags;
delete types.flags;

Alternatively, assuming .total to be the sum of all the other values, then it can be accumulated on the fly :

var types = {
    'helpful': 0,
    'declined': 0,
    'disputed': 0,
    'aged': 0,
    'total': 0
};

Object.keys(types).slice(0, -1).forEach(function(key) { // .slice(0, -1) removes 'total' from the array of keys 
    $("td > a:contains('" + key + "')").parent().prev().each(function () {
        types[key] += parseInt($(this).text().replace(',',''));
        types.total += types[key];
    });
});
\$\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.