current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I am setting a few vars in JavaScript. I typically like to make my code really readable so that it's easy to go back and edit. I have a few questions about the code below:

1) Is this is a good way to break up the setting of the vars? The comment seems redundant, but it seems to really help break up the code into relevant chunks.

2) When I set a var like this ~~ $IPPWrapper = $("div.iqPaginItemsPerPageWrapper") ~~ I always include the HTML element, in this case the "div". Is this overkill? Is there a performance hit or benefit? I know in the CSS best practices, it's frowned upon.

3) I need to create an array, but I don't know the best type to use. Basically, I am just storing messages that I will use later in popups. Is there a more common or better way to create this array?

Thanks in advance for any advice or helpful comments.

// GENERAL VARS
var UpRate = 250,
    DownRate = UpRate,
    OpenHeight = "25px";

// SET IPP VARS
var $IPPWrapper = $("div.iqPaginItemsPerPageWrapper"),
    $IPPDefault = $IPPWrapper.children("div.iqPaginDefault"),
    $IPPNonDefault = $IPPWrapper.children("div.iqPaginNonDefault"),
    $IPPOptions = $IPPWrapper.children("div.ItemsPerPage"),
    $IPPValue = $("span#ItemsPerPageValue");

// SET SB VARS
var $SBWrapper = $("div.iqPaginSortByWrapper"),
    $SBDefault = $SBWrapper.children("div.iqPaginDefault"),
    $SBNonDefault = $SBWrapper.children("div.iqPaginNonDefault"),
    $SBOptions = $SBWrapper.children("div.SortBy"),
    $SBValue = $("span#SortByValue");

// CREATE SB TEXT ARRAY
var SBTextArray = new Array();
    SBTextArray[1] = "Company Name A - Z";
    SBTextArray[2] = "Company Name Z - A";
    SBTextArray[3] = "Product Name A - Z";
    SBTextArray[4] = "Product Name Z - A";
    SBTextArray[5] = "Model Number A - Z";
    SBTextArray[6] = "Model Number Z - A";

EDIT ~~ Is this a better way of coding the above array?

// CREATE SB TEXT ARRAY
var SBTextArray = [ 
        "Company Name A - Z", "Company Name Z - A",
        "Product Name A - Z", "Product Name Z - A",
        "Model Number A - Z", "Model Number Z - A",
    ];
share|improve this question
1  
don't use new Array() go for the array literal var SBTextArray = ["Company Name A - Z","Company Name Z - A", ....]; read more here –  rlemon Sep 5 '12 at 16:33
    
@rlemon, thanks for the link. I have this correct now, right? –  Evik James Sep 5 '12 at 17:04
2  
yes, the array is now zero index'd - if you want to set the keys you will use an object. Arrays should be zero index'd in js. –  rlemon Sep 5 '12 at 17:33
    
You can test for performance yourself - check out jsperf.com. –  delete me Sep 5 '12 at 23:55

2 Answers 2

up vote 4 down vote

Your code is great so far. The only tip I can think of is to wrap grouped variables inside a namespace / singleton. This would make it easier to debug and perform operations of group of related elements.

    // GENERAL VARS
var UpRate = 250,
    DownRate = UpRate,
    OpenHeight = "25px";

// SET IPP VARS
var $IPP = {};
$IPP.Wrapper = $("div.iqPaginItemsPerPageWrapper");
$IPP.Default = $IPP.Wrapper.children("div.iqPaginDefault");
$IPP.NonDefault = $IPP.Wrapper.children("div.iqPaginNonDefault");
$IPP.Options = $IPP.Wrapper.children("div.ItemsPerPage");
$IPP.Value = $("span#ItemsPerPageValue");

// SET SB VARS
var $SB = {};
$SB.Wrapper = $("div.iqPaginSortByWrapper");
$SB.Default = $SB.Wrapper.children("div.iqPaginDefault");
$SB.NonDefault = $SB.Wrapper.children("div.iqPaginNonDefault");
$SB.Options = $SB.Wrapper.children("div.SortBy");
$SB.Value = $("span#SortByValue");

// CREATE SB TEXT ARRAY
var SBTextArray = [];
SBTextArray[1] = "Company Name A - Z";
SBTextArray[2] = "Company Name Z - A";
SBTextArray[3] = "Product Name A - Z";
SBTextArray[4] = "Product Name Z - A";
SBTextArray[5] = "Model Number A - Z";
SBTextArray[6] = "Model Number Z - A";

1) Use a namespace for related elements and functionality.

2) It depends. In most cases it doesn't matter unless if you different tags with the same classes. Like p.class1 and dd.class1. More info here

3) You could just use an object literal to store the message by a reference name. Something like this.

var listing = { az: {}, za: {} };
listing.az.company = "Company Name A - Z";
listing.az.product = "Product Name A - Z";
listing.az.model = "Model Number A - Z";
listing.za.company = "Company Name Z - A";
listing.za.product = "Product Name Z - A";
listing.za.model = "Model Number Z - A";

I think your current code is good enough though.

share|improve this answer
    
Larry, that looks interesting. Do you have a link to a good article on the concept of using the namespace like you are using? –  Evik James Sep 5 '12 at 16:59
1  
"Namespacing in javascript" elegantcode.com/2011/01/26/basic-javascript-part-8-namespaces –  Larry Battle Sep 5 '12 at 17:01
up vote 0 down vote

In javascript notmally Classes are CamelCase constants are ALLCAPS

These look like constants to me:

// GENERAL VARS
var UpRate = 250,
    DownRate = UpRate,
    OpenHeight = "25px";

and as such I would change them to all caps

// GENERAL VARS
var UPRATE = 250,
    DOWNRATE = UPRATE,
    OPENHEIGHT = "25px";

Some people like CONSTANTS_WITH_UNDERSCORES and some don't. That's really up to you but this way allows you to easily distinguish between classes and constants.

Regular variables

var $IPPWrapper = $(....

var SBTextArray = [ ... ] ;

should either be $lowerCase or just lowerCase. Something like:

var $ippWrapper = $(....

var sbTextArray = [ ... ] ;
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.