3
\$\begingroup\$

I created the Stackapp IGB Stack Flair a while ago and I think it's a good time for a revision now. Is there anything that can be done better, shorter, more elegant, more performant, etc.?

I thought of using something like a map/associative array for the SE sites in addSiteLinkFor(), for instance. [This has been implemented. --G.B., April 21, 2017]

Please note:

  • My indent style is Whitesmiths adapted by the opening brace on the same line to save vertical space here.
  • The spaces after/before opening/closing parenthesis are intentional.
  • 120 chars line length are intentional (I can share a GreaseMonkey script that overcomes this width limit of SE's sites if you want.)

/*
 * IGB Stack Flair – A dynamic flair for Stack Exchange sites 
 * Copyright (C) 2017 Gerold Broser ([email protected])
 * 
 * This program is free software: you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation, either version 3 of the License, or
 * (at your option) any later version.
 * 
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 * GNU General Public License for more details.
 * 
 * You should have received a copy of the GNU General Public License
 * along with this program. If not, see <http://www.gnu.org/licenses/>.
 */
/*
 * FIXME:
 *    - use jQuery
 *    - cookie caching doesn't seem to work, perhaps a Firefox add-on issue
 * TODO:
 *    - test in browsers other than Firefox, Opera, Chrome
 *    - add further SE sites to 'sites' prior to 'addSiteLinkFor()'
 *    Future:
 *    - add i18n
 */

let usersAJAX = null    
let userAJAX = null
let users = null
let seUser = null // had to be renamed from 'user' since otherwise it conflicts with Wikipedia
let cookieExpirationTime = null
let usingCachedInfo = true

/** A dynamic flair for Stack Exchange sites inspired by Stack Exchange's native User Flairs 
 *  ( http://stackoverflow.com/users/flair/ ) with advanced features:
 * 
 * ● Including Stack Exchange sites with more than 101 reputation (compared to 200).
 * ● Detailed reputation summary (compared to K and M abbreviations).
 * ● Detailed statistics about Questions, Answers, Reputation, Gold, Silver and Bronze badges for each included site
 *   (compared to <null>)
 * ● Updated in real-time (compared to 24–36 hrs).
 * 
 * @version 17.4.20
 * @param elementId The HTML element's id IGB Stack Flair is to be inserted in. See 'igbstackflair.html' for an example.
 * @param networkProfileId As it can be seen under 'Network Profile' at top-right on your SE site's profile page.
 * @param transformScale Scale factor for resizing IGB Stack Flair.
 * @param retentionPeriod The period in hours that IGB Stack Flair's stores site information in its internal cache.
 *                        Note that SE has a daily limit of 300 retrievals per (unauthenticated) user for its databases.
 *                        ( see https://api.stackexchange.com/docs/throttle )
 *                        Note that during the retention period no current data are displayed, of course.
 * @param maxSiteLinksCount Number of site links that are displayed in IGB Stack Flair at a max.
 * @param maxSiteLinksPerLine Number of site links that are displayed in one line of IGB Stack Flair at a max.
 * 
 * @see https://stackapps.com/questions/5004/igb-stack-flair-a-dynamic-flair-for-stack-exchange-sites
 * @see https://sourceforge.net/projects/igbstackflair/
 */
// From http://stackoverflow.com/questions/11796093/named-parameters-in-javascript/11796776#11796776
//   by http://stackoverflow.com/users/218196/felix-kling
function igbStackFlair( {
            elementId='igbstackflair', 
            networkProfileId=1937747, 
            transformScale=1.0, 
            retentionPeriod=1, 
            maxSiteLinksCount=99, 
            maxSiteLinksPerLine=8 } = {} ) {

    users = JSON.parse( cookie( 'stackUsers' + networkProfileId ) )
    user = JSON.parse( cookie( 'stackUser' + networkProfileId ) )
    cookieExpirationTime = cookie( 'stackUser' + networkProfileId + 'Expiration' )
    if ( users === null || seUser === null ) {
        cookieExpirationTime = nowPlusOffsetToUtcString( retentionPeriod )
        usingCachedInfo = false

        if ( window.XMLHttpRequest ) { // Google Chrome, Mozilla Firefox, Opera, Safari, IE 10
            usersAJAX = new XMLHttpRequest()
            userAJAX = new XMLHttpRequest()
            }
        else if ( window.ActiveXObject ) { // Internet Explorer 9 and lower
            try {
                usersAJAX = new ActiveXObject( 'Msxml2.XMLHTTP.6.0' )
                userAJAX = new ActiveXObject( 'Msxml2.XMLHTTP.6.0' )
                }
            catch(e) {
                try {
                    usersAJAX = new ActiveXObject( 'Msxml2.XMLHTTP.3.0' )
                    userAJAX = new ActiveXObject( 'Msxml2.XMLHTTP.3.0' )
                    }
                catch(e) {
                    console.error( "igbstackflair.js:\n" + e )
                    document.getElementById( elementId ).innerHTML = "igbstackflair.js:<br />" + e
                    }
                }
            }
        if ( usersAJAX == null || userAJAX == null ) {
            alert( "igbstackflair.js: Your browser doesn't support AJAX." )
            return false
            }
        } // if ( no cookies )

    document.getElementById( elementId ).innerHTML = 
        '<table cellspacing="0" cellpadding="0" style="transform:scale(' + transformScale +
                '); background-color:#e0eaf1; font-family:Arial,Liberation Sans,DejaVu Sans,sans-serif;">' +
            '<tr><td rowspan="3" style="padding:2px"><a id="stackProfileImageLink" target="_blank">' +
                        '<img id="stackProfileImage" width="72" height="72" style="text-align:center; vertical-align:middle;" /></a></td>' +
                    '<td>&nbsp;<strong><a id="networkProfileLink" style="text-decoration:none;" target="_blank"></a><strong></td>' +
                    '<td style="text-align:right;">' +
                        '<img id="stackInfo" src="https://upload.wikimedia.org/wikipedia/commons/3/33/Info_icon_002.svg" ' +
                            'alt="Info" title="Info" width="14" height="14" style="vertical-align:top;"/></td></tr>' +
            '<tr><td colspan="2">&nbsp;<strong id="stackReputation">0</strong>&nbsp;' +
                        ' <span style="color:gold">●</span>&thinsp;<span id="stackGold">0</span>' +
                        ' <span style="color:silver">●</span>&thinsp;<span id="stackSilver">0</span>' +
                        ' <span style="color:darkgoldenrod">●</span>&thinsp;<span id="stackBronze">0</span>&nbsp;</td></tr>' +
            '<tr><td id="stackSiteLinks" colspan="2">&nbsp;</td></tr></table>'

    document.getElementById( 'stackInfo' ).title = 
        ( usingCachedInfo ? "Cached data used. Data will be updated at " : "Retrieved data used. Data will be cached until " ) +
            new Date( cookieExpirationTime ).toLocaleString()

    if ( users !== null ) {
        addFlair( networkProfileId, retentionPeriod, maxSiteLinksCount, maxSiteLinksPerLine )
        }   
    else {  
        let usersURL = 'https://api.stackexchange.com/2.2/users/' +
                       networkProfileId + '/associated?filter=!4XDqt9bmJLeeGDHz_Ky)Dc9Buv'
        usersAJAX.open( 'GET', usersURL, true )
        usersAJAX.onreadystatechange = function() {
            if ( this.readyState == 4 ) {
                if ( this.status == 200 ) {
                    users = JSON.parse(this.responseText)
                    document.cookie = "stackUsers" + networkProfileId + "=" + encodeURIComponent(JSON.stringify(users)) +
                                      ";expires=" + cookieExpirationTime
                    document.cookie = "stackUser" + networkProfileId + "Expiration=" + cookieExpirationTime +
                                      ";expires=" + cookieExpirationTime

                    addSites( networkProfileId, retentionPeriod, maxSiteLinksCount, maxSiteLinksPerLine )
                    } // if (status OK)
                else {
                    console.error( "igbstackflair.js: " + usersURL + "\n" + this.statusText + ": " + this.responseText )
                    document.getElementById( elementId )
                        .innerHTML = "igbstackflair.js: " + usersURL + "<br />" + this.statusText + ": "+this.responseText
                    }
                } // if ( status ready )
            } // onreadystatechange function()
        usersAJAX.send(null)
        }   
    } // igbStackFlair()

/** Adds SE site items.
 * 
 * @param networkProfileId°
 * @param retentionPeriod°
 * @param maxSiteLinksCount°
 * @param maxSiteLinksPerLine°
 * 
 * °See function igbStackFlair() above for descriptions of these parameters.
 */
function addSites( networkProfileId, retentionPeriod, maxSiteLinksCount, maxSiteLinksPerLine ) {

    let networkProfileLink = 'http://stackexchange.com/users/' + users.items[0].account_id + '?tab=accounts'
    users.items.sort(function(u1, u2) { // sort sites by descending reputation
        return u2.reputation - u1.reputation
        }) // sort function(...)

    if (seUser === null) {
        let userURL = 'https://api.stackexchange.com/2.2/users/' + users.items[0].user_id + '?site=' +
                      users.items[0].site_url.substring(7) + '&filter=!*MxNUg6fdlYu_0rC&'
        userAJAX.open( 'GET', userURL, true)
        userAJAX.onreadystatechange = function() {
            if ( this.readyState == 4 ) {
                if ( this.status == 200 ) {
                    seUser = JSON.parse(this.responseText).items[0]
                    document.cookie = "stackUser" + networkProfileId + "=" + encodeURIComponent(JSON.stringify(user)) +
                                      ";expires=" + cookieExpirationTime
                    addLinks( networkProfileLink, maxSiteLinksCount, maxSiteLinksPerLine )
                    } // if ( status OK )
                else {
                    console.error( "igbstackflair.js: " + userURL + "\n" + this.statusText + ": " + this.responseText )
                    document.getElementById( elementId )
                        .innerHTML = "igbstackflair.js: " + userURL + "<br />" + this.statusText + ": " + this.responseText
                    }
                } // if (status ready)
            } // onreadystatechange function()
            userAJAX.send( null )
        }
    else { // seUser !== null
        addLinks( networkProfileLink, maxSiteLinksCount, maxSiteLinksPerLine )
        }
    document.getElementById( 'stackInfo' )
        .title += " - " + users.quota_remaining + " of " + users.quota_max + " requests remaining"  
    } // addSites()

/** Adds links to the profile image and to site items.
 * 
 * @param networkProfileLink Link to the StackExchange Network Profile.
 * @param maxSiteLinksCount°
 * @param maxSiteLinksPerLine°
 * 
 * °See function igbStackFlair() above for descriptions of these parameters.
 */
function addLinks( networkProfileLink, maxSiteLinksCount, maxSiteLinksPerLine ) {

    document.getElementById( 'stackProfileImageLink' ).href = networkProfileLink
    with ( document.getElementById( 'stackProfileImage' ) ) {
        src = seUser.profile_image
        alt = "profile image"
        title = seUser.display_name
    }
    with ( document.getElementById( 'networkProfileLink' ) ) {
        href = networkProfileLink
        alt = seUser.display_name
        innerHTML = seUser.display_name
    }
    for ( let u = 0; u < ( maxSiteLinksCount < users.items.length ? maxSiteLinksCount : users.items.length ); u++ ) {
        if ( users.items[u].reputation > 101 )
            addSiteLinkFor( users.items[u], maxSiteLinksCount, maxSiteLinksPerLine )
        }
    document.getElementById( 'stackReputation' ).innerHTML =
        parseInt( document.getElementById( 'stackReputation' ).innerHTML ).toLocaleString('en') 
    } // addLinks()

let reputation = 0
let gold = 0
let silver = 0
let bronze = 0
let siteLinksPerLineSoFar = 0
let sites = { 
        academia:{short:'AC'},
        askubuntu:{short:'ask'},
        codegolf:{short:'PCG'},
        codereview:{short:'CR'},
        crypto:{short:'crpt'},
        cs:{short:'CS'},
        cstheory:{short:'CST'},
        dba:{short:'DBA'},
        electronics:{short:'elec'},
        english:{short:'EN'},
        german:{short:'DE'},
        mechanics:{short:'Mech'},
        meta:{short:'meta'},
        physics:{short:'Ph'},
        serverfault:{short:'SF'},
        softwareengineering:{short:'SE'},
        softwarerecs:{short:'SW'},
        stackapps:{short:'StA'},
        stackoverflow:{short:'SO'},
        superuser:{short:'SU'},
        unix:{short:'UNIX'},
        ux:{short:'UX'},
        webapps:{short:'apps'},
        workplace:{short:'work'}
        }

/** Adds links to site items.
 * 
 * @param siteUser The SE site's user.
 * @param maxSiteLinksCount°
 * @param maxSiteLinksPerLine°
 * 
 * °See function igbStackFlair() above for descriptions of these parameters.
 */
function addSiteLinkFor( siteUser, maxSiteLinksCount, maxSiteLinksPerLine ) {

    document.getElementById( 'stackReputation' ).innerHTML = reputation += siteUser.reputation
    document.getElementById( 'stackGold' ).innerHTML = gold += siteUser.badge_counts.gold
    document.getElementById( 'stackSilver' ).innerHTML = silver += siteUser.badge_counts.silver
    document.getElementById( 'stackBronze' ).innerHTML = bronze += siteUser.badge_counts.bronze 

    let siteLinks = document.getElementById( 'stackSiteLinks' )
    let siteLink = document.createElement( 'a' )
    with ( siteLink ) {
        target = '_blank'
        href = siteUser.site_url + '/users/' + siteUser.user_id
        style.textDecoration = 'none'

        let siteImage = document.createElement( 'img' )
        if ( siteUser.site_url.indexOf('meta') == 8 ) {
            siteImage.src = 'https://meta.stackexchange.com/content/Sites/stackexchangemeta/img/icon-16.png'
            siteImage.alt = sites['meta'].short
        }
        else if (siteUser.site_url.indexOf('stackoverflow') == 7 ) {
            siteImage.src = 'https://cdn.sstatic.net/Sites/stackoverflow/img/icon-16.png'
            siteImage.alt = sites['stackoverflow'].short
        }
        else  {
            siteServer = siteUser.site_url.substr( 8, siteUser.site_url.indexOf('.') - 8 )
            siteImage.src = 'https://cdn.sstatic.net/Sites/' + siteServer + '/img/icon-16.png'
            siteImage.alt = sites[siteServer].short
        }


        // Note: No space between ':' and counts since neither '&nbsp;' nor character U+00A0 (NO-BREAK SPACE)
        //       are rendered correctly in title (in FF)
        siteImage.title = decodeURIComponent(siteUser.site_name) + " - Questions:" + parseInt( siteUser.question_count ).toLocaleString('en') +
                          "  Answers:" + parseInt( siteUser.answer_count ).toLocaleString('en') + "  Reputation:" + parseInt( siteUser.reputation ).toLocaleString('en')    + 
                              "  Gold:" + parseInt( siteUser.badge_counts.gold ).toLocaleString('en') + "  Silver:" + parseInt( siteUser.badge_counts.silver ).toLocaleString('en') +
                              "  Bronze:" + parseInt( siteUser.badge_counts.bronze ).toLocaleString('en')
        siteImage.style.verticalAlign = 'middle'
        appendChild( siteImage )
        } // with ( siteLink )
    siteLinks.appendChild( siteLink )

    siteLinks.innerHTML += "&nbsp;"
    if ( ++siteLinksPerLineSoFar >= maxSiteLinksPerLine && maxSiteLinksCount > maxSiteLinksPerLine ) {
         siteLinks.innerHTML += "<br />&nbsp;"
         siteLinksPerLineSoFar = 0
        }
    } // addSiteLinkFor( siteUser )

/** TODO comment
 * 
 * @param key TODO comment
 */
// From http://stackoverflow.com/questions/5639346/what-is-the-shortest-function-for-reading-a-cookie-by-name-in-javascript/25473576#25473576
//   by http://stackoverflow.com/users/996638/simon-steinberger
function cookie( key ) {
    let value = document.cookie.match( '(^|;) ?' + key + '=([^;]*)(;|$)' )
    return value ? decodeURIComponent( value[2] ) : null
    }

/** Returns an UTC string containing the current time plus the given offset.
 * 
 * @param offsetHours TODO comment
 * @returns An UTC string containing the current time plus the given offset.
 */
function nowPlusOffsetToUtcString( offsetHours ) {
    let date = new Date()
    date.setTime( date.getTime() + ( offsetHours * 3600000 ) )
    return date.toUTCString()
    }
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Take a look at these two snippets. This one...

    if ( siteUser.site_url.indexOf('meta') == 8 ) {
        siteImage.src = 'https://meta.stackexchange.com/content/Sites/stackexchangemeta/img/icon-16.png'
        siteImage.alt = sites['meta'].short
    }
    else if (siteUser.site_url.indexOf('stackoverflow') == 7 ) {
        siteImage.src = 'https://cdn.sstatic.net/Sites/stackoverflow/img/icon-16.png'
        siteImage.alt = sites['stackoverflow'].short
    }
    else  {
        siteServer = siteUser.site_url.substr( 8, siteUser.site_url.indexOf('.') - 8 )
        siteImage.src = 'https://cdn.sstatic.net/Sites/' + siteServer + '/img/icon-16.png'
        siteImage.alt = sites[siteServer].short
    }

and

if (seUser === null) {
    let userURL = 'https://api.stackexchange.com/2.2/users/' + users.items[0].user_id + '?site=' +
                  users.items[0].site_url.substring(7) + '&filter=!*MxNUg6fdlYu_0rC&'
    userAJAX.open( 'GET', userURL, true)
    userAJAX.onreadystatechange = function() {
        if ( this.readyState == 4 ) {
            if ( this.status == 200 ) {
                seUser = JSON.parse(this.responseText).items[0]
                document.cookie = "stackUser" + networkProfileId + "=" + encodeURIComponent(JSON.stringify(user)) +
                                  ";expires=" + cookieExpirationTime
                addLinks( networkProfileLink, maxSiteLinksCount, maxSiteLinksPerLine )
                } // if ( status OK )
            else {
                console.error( "igbstackflair.js: " + userURL + "\n" + this.statusText + ": " + this.responseText )
                document.getElementById( elementId )
                    .innerHTML = "igbstackflair.js: " + userURL + "<br />" + this.statusText + ": " + this.responseText
                }
            } // if (status ready)
        } // onreadystatechange function()
        userAJAX.send( null )
    }
else { // seUser !== null
    addLinks( networkProfileLink, maxSiteLinksCount, maxSiteLinksPerLine )
    }

that one. Compare the indentation.

You're not being consistent with indentation. You've gone for a style I've never seen used before - fine by me. But if you don't follow it up by being consistent, it just hurts the code. Get a code formatter that can format the code in the way you prefer consistently, and use it.

Oh, by the way...

    else  {

That's a double space.

Specifically:

    if ( siteUser.site_url.indexOf('meta') == 8 ) {
        siteImage.src = 'https://meta.stackexchange.com/content/Sites/stackexchangemeta/img/icon-16.png'
        siteImage.alt = sites['meta'].short
    }

vs

            if ( this.status == 200 ) {
                seUser = JSON.parse(this.responseText).items[0]
                document.cookie = "stackUser" + networkProfileId + "=" + encodeURIComponent(JSON.stringify(user)) +
                                  ";expires=" + cookieExpirationTime
                addLinks( networkProfileLink, maxSiteLinksCount, maxSiteLinksPerLine )
                } // if ( status OK )

Is about your braces, and this...

    else if (siteUser.site_url.indexOf('stackoverflow') == 7 ) {

vs

            if ( this.status == 200 ) {

You're not being consistent with spaces for if-statements.

And your indentation style is dangerous:

if (seUser === null) {
    let userURL = 'https://api.stackexchange.com/2.2/users/' + users.items[0].user_id + '?site=' +
                  users.items[0].site_url.substring(7) + '&filter=!*MxNUg6fdlYu_0rC&'
    userAJAX.open( 'GET', userURL, true)
    userAJAX.onreadystatechange = function() {
        if ( this.readyState == 4 ) {
            if ( this.status == 200 ) {
                seUser = JSON.parse(this.responseText).items[0]
                document.cookie = "stackUser" + networkProfileId + "=" + encodeURIComponent(JSON.stringify(user)) +
                                  ";expires=" + cookieExpirationTime
                addLinks( networkProfileLink, maxSiteLinksCount, maxSiteLinksPerLine )
                } // if ( status OK )
            else {
                console.error( "igbstackflair.js: " + userURL + "\n" + this.statusText + ": " + this.responseText )
                document.getElementById( elementId )
                    .innerHTML = "igbstackflair.js: " + userURL + "<br />" + this.statusText + ": " + this.responseText
                }
            } // if (status ready)
        } // onreadystatechange function()
        userAJAX.send( null )
    }
else { // seUser !== null
    addLinks( networkProfileLink, maxSiteLinksCount, maxSiteLinksPerLine )
    }

snip...

if (seUser === null) {
    let userURL = 'https://api.stackexchange.com/2.2/users/' + users.items[0].user_id + '?site=' +
                  users.items[0].site_url.substring(7) + '&filter=!*MxNUg6fdlYu_0rC&'
    userAJAX.open( 'GET', userURL, true)
    userAJAX.onreadystatechange = function() {
        if ( this.readyState == 4 ) {
            if ( this.status == 200 ) {
                //snip
                //snip
                //snip
                } // if ( status OK )
            else {
                //snip
                //snip
                }
            } // if (status ready)
        } // onreadystatechange function()
        userAJAX.send( null )
    }
else { // seUser !== null
    // snip
    }

snipping again...

if (seUser === null) {
    let userURL = 'https://api.stackexchange.com/2.2/users/' + users.items[0].user_id + '?site=' +
                  users.items[0].site_url.substring(7) + '&filter=!*MxNUg6fdlYu_0rC&'
    userAJAX.open( 'GET', userURL, true)
    userAJAX.onreadystatechange = function() {
        if ( this.readyState == 4 ) {
            if ( this.status == 200 ) {
                } // if ( status OK )
            else {
                }
            } // if (status ready)
        } // onreadystatechange function()
        userAJAX.send( null )
    }
else { // seUser !== null
    // snip
    }

This line, userAJAX.send( null ), when do you think it's being executed?

I dunno, let's apply OTBS (from the same wikipedia page)!

if (seUser === null) {
    let userURL = 'https://api.stackexchange.com/2.2/users/' + users.items[0].user_id + '?site=' +
                  users.items[0].site_url.substring(7) + '&filter=!*MxNUg6fdlYu_0rC&'
    userAJAX.open( 'GET', userURL, true)
    userAJAX.onreadystatechange = function() {
        if ( this.readyState == 4 ) {
            if ( this.status == 200 ) {
            } // if ( status OK )
            else {
            }
        } // if (status ready)
    } // onreadystatechange function()
        userAJAX.send( null )
}
else { // seUser !== null
    // snip
}

Oh, so it's AFTER the function!

And, you know, we kinda could have spotted this already:

        } // onreadystatechange function()
        userAJAX.send( null )

But I'm of the opinion that you don't need to add a comment for the ending braces of functions or the sort, because, well, that's what your brace-style is for.

My point is, due to inconsistent formatting and a weird brace style, you have managed to improperly indent your code and caused confusion about which scope a specific line is executed.

I had trouble looking at your code. The lack of semicolons and the weird formatting style made it hard for me to properly look at the content. So this is a review just about the formatting.


Get a formatter which works for you and make use of it. Uniform codestyle means people can adjust. Haphazard application of obscure formatting makes it hard if not impossible for people to adjust.

Get rid of the comments denoting the end of a scope. If you find you need them anyway, switch brace-styles until you find something that causes you not to need such end-of-scope braces. If it fits on your screen, you don't need such comments. If it doesn't fit on your screen, your function might be too large. (IntelliJ shows me 55 lines with the debugger closed at the moment, 31 with the debugger open).

The thing about those comments is, if they're close to the start of the scope, the comment is useless.

if ( blah blah blah blah blah blah == 0 ) {
    blah blah blah blah blah
    } //end if

That's a useless comment.

function checkTheThings ( blah blah blah blah blah blah ) {
    if ( blah blah blah blah blah blah == 0 ) {
        blah blah blah blah blah
        if ( blah blah blah blah blah blah == 0 ) {
            if ( blah blah blah blah blah blah == 0 ) {
                blah blah blah blah blah
                }
            blah blah blah blah blah
            }
        }
    if ( blah blah blah blah blah blah == 0 ) {
        blah blah blah blah blah
        if ( blah blah blah blah blah blah == 0 ) {
            if ( blah blah blah blah blah blah == 0 ) {
                blah blah blah blah blah
                }
            blah blah blah blah blah
            }
        }
    if ( blah blah blah blah blah blah == 0 ) {
        blah blah blah blah blah
        if ( blah blah blah blah blah blah == 0 ) {
            if ( blah blah blah blah blah blah == 0 ) {
                blah blah blah blah blah
                }
            blah blah blah blah blah
            }
        }
    } //end function checkTheThings

This comment risks getting outdated. Imagine you rename the function, or for an if statement, you change the contents. You also have to update the comment. That's duplicate work. You don't want to have to do duplicate work because if you forget to do the duplicate work then the comments start getting out of sync and your assumptions (that the comments are correct) would start to go wrong, which is a pain.


Oh and ...

let seUser = null // had to be renamed from 'user' since otherwise it conflicts with Wikipedia

That's a really scary comment. Are you sure you can't put your code in a private scope so you're not affected by external variables?


if ( siteUser.site_url.indexOf('meta') == 8 ) {
else if (siteUser.site_url.indexOf('stackoverflow') == 7 ) {

I think you should add a comment for what you're trying to do here. 8 and 7 smell like magic numbers, their meaning is not directly obvious.

\$\endgroup\$
4
  • \$\begingroup\$ In this review: I hate your code style but I'm not allowed to say such things so I bash your consistency instead in the hope you switch to a more commonly used style which is better supported by formatting tools \$\endgroup\$
    – Pimgd
    Commented Apr 21, 2017 at 12:54
  • \$\begingroup\$ What a pity that you have such negative feelings like hate. Thank you for bashing and, yes, thank you for your answer, too. I don't have the time right atm but I'm going to study it and take the things you mention into consideration. The issue with the inconsistent indentation bites me, too. Both Eclipse's and Amatera's JS editor (on Neon.3, v4.6.3) throw exceptions frequently here. Preferably at Ctrl+Shift+F formatting or saving with an enabled Format automatically save action. \$\endgroup\$ Commented Apr 21, 2017 at 13:08
  • \$\begingroup\$ @GeroldBroser Sorry, I just don't like the formatting style you're using. I've tried to channel this hate into a productive answer instead of bashing where possible. \$\endgroup\$
    – Pimgd
    Commented Apr 21, 2017 at 13:13
  • \$\begingroup\$ Do you know a JS editor plugin for Eclipse that has a formatter like the one in JDT that is more stable and can be enabled as a save action (though the last is not a must)? "The lack of semicolons"? Have a look at this answer where I was told: "You're using many semicolons. JavaScript has a nice feature called automation semicolon insertion, which means you almost don't need to use semicolons at all, with just a few exceptions.". \$\endgroup\$ Commented Apr 22, 2017 at 4:51

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.