Could someone critique my Javascript?

[mrgstiffler]

I'm nowhere close to a professional or expert or anything, but I'm trying to get better and employ newer more modern techniques. I don't work with anyone else on my projects so I don't have anyone to look at my code and make suggestions. Any input you could provide would be greatly appreciated.

Basically, mainLoad() is called on document ready and it generates a listing of active 911 calls, along with the units assigned to the calls, from a JSON string. Then it updates the container, waits 10 seconds and does it again.

The listing looks like this (This is dummy data):

Javacript:
http://pastebin.com/PtHwASCV

 

[KingKaeru]

Just gave your script a quick glance and the two things that stood out were:

1. overuse of comments.
2. overly long functions.

Example:
The function can probably be renamed from checkExist to existsOnScreen. This should be clear enough where you wont need any of the comments.

        /* Check to see if the call is already on the screen. */
        checkExist: function() {
            if ($('#' + this.callData.Call_Number).length === 1) {
                return true;
                //Element exists on screen.
            } else {
                return false;
                //Element does not exist on screen.
            }
        },

        existsOnScreen: function() {
            return ($('#' + this.callData.Call_Number).length === 1);
        },

                if (!callObj.checkExist()) {
                    //Check to see if call is already on the screen.
                    //TODO Add addMarker code.
                }

                if (!callObj.existsOnScreen()) {
                    addMarker();
                }

 

[mrgstiffler]

OK, great! That's the kind of feedback I need. Makes perfect sense! Thank you very much.

EDIT: I just noticed that a lot of my comment were moved down a line by the autoformat tool. Most of them are inline comments instead of having their own lines.

 

[Lunknumb]

Rule of thumb: Use comments when you need to explain why the code does what it does.

If you need comments to explain what the code does, then you should probably take a second look at it.

 

[ShoeLace]

Over using comments is better than not commenting anything but you do seem to go a little overboard.

Your comments also look like you're writing them in such a way that you expect the person reading the code to not have any experience with jquery. If you're learning jquery as you go then it's great to make these comments but if you're somewhat knowledgeable of the framework then most of your comments could be omitted.

I tend to make comments above a certain method/function explaining what the purpose of the function is and leave it at that. If there's some hairy logic or some type of formula I'll write out line by line comments for each step if I think it's something that could be forgotten in the future or isn't 100% straight forward.

Overall your code is pretty solid. Also you're following the same formatting for both variable names and spacing which is always nice. These are minor things but are pretty important (to me at least).

 

[FreiDOg]

Just gave your script a quick glance and the two things that stood out were:

1. overuse of comments.
2. overly long functions.

Example:
The function can probably be renamed from checkExist to existsOnScreen. This should be clear enough where you wont need any of the comments.

The question is really the use; javascript is of course interrupted on the browser, everything including comments and white space is sent down and then parsed. If you're serving 1000 page hits a day, I'm not sure how much I would care about that... If you're handling 1000 unique requests a minute, you probably want to run it through a tool that removes comments, white-space and does other space/bandwidth saving measures (in addition to some of KingKaeru's code change suggestions). and certainly make sure you've moved all this to it's own .js file so the browsers can cache the code even if the data has changed.

 

[jiminator]

you mean interpreted. and I think some browsers may actually compile it in some form. I also don't like comment abuse. if you use good variable and function names and indenting you are 95% there. you can use comments to explain "why" some things have to happen but otherwise they can be useless clutter that makes the code hard to read.

 

[mrgstiffler]

Over using comments is better than not commenting anything but you do seem to go a little overboard.

Your comments also look like you're writing them in such a way that you expect the person reading the code to not have any experience with jquery. If you're learning jquery as you go then it's great to make these comments but if you're somewhat knowledgeable of the framework then most of your comments could be omitted.

Yeah, the idea is to give the finished code to other agencies that may have little to no IT support, but I can see how even then that much commenting isn't necessary.

I really appreciate all the advice!