Skip to main content
added 272 characters in body
Source Link
Pimgd
  • 22.3k
  • 5
  • 66
  • 144

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.


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.

Source Link
Pimgd
  • 22.3k
  • 5
  • 66
  • 144

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?