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.

I wanted to be able to consistently label an arbitrary number of objects on my site. It's pretty simple, but I if there's a more clever way...

// Usage:
// ColorTable.getColor('my link name');
ColorTable = {
  '_colors' : [
    '#F4BB2E',
    '#2DE656',
    '#66CCFF',
    '#6666FF',
    '#CC66FF',
    '#FF6FCF',
    '#FC4048'
   ],
  'tableSize' : function() { return this['_colors'].length; },
  'nextIndex' : function() {
    if (localStorage["ColorTable:_index"]===undefined) {
        localStorage["ColorTable:_index"] = -1;
    }
    rv = (parseInt(localStorage["ColorTable:_index"]) + 1) % this.tableSize();
    localStorage["ColorTable:_index"] = rv;
    return rv;
  },
  'getIndex' : function(key) {
    if (!localStorage["ColorTable:" + key]) {
      localStorage["ColorTable:" + key] = this.nextIndex();
    }
    return localStorage["ColorTable:" + key];
  },
  'setIndex' : function(key, index) {
    if (index >= 0 && index < this.tableLength()) {
      localStorage["ColorTable:" + key] = index;
      return localStorage["ColorTable:" + key];
    } else {
      return false;
    }
  },
  'getColor' : function(key) {
    return this['_colors'][this.getIndex(key)];
  }
};
share|improve this question

1 Answer 1

up vote 1 down vote accepted

Minor nitpicks:

None of you keywords are reserved words (especially if it starts with an underscore!) so encasing them in single-quotes is superfluous.

ColorTable.tableSize is an example of the smurf-naming convention (see item 21). Drop the second instance of table to become ColorTable.size

For setIndex you return two different datatypes: index on success, and false for failure. Since index is an arguements known to external code, what purpose does this serve? For consistency, you should return true/false for pass/fail if you have to. If you have to, how about a single point-of-return? You'd have to introduce a variable, and two more LOCs, but... single point-of-return!

But why not throw an exception? The existing mixed-type return suggests that calling code doesn't use the return value in any meaningful way.

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.