D41d8cd98f00b204e9800998ecf8427e

Hi all, first time poster here.

Can you take a look at the following at suggest ways to refactor please :) Simple script, just looks a bit bloated to me.

Cheers,
Frog

var d = new Date();
var y = new Array();
	y[0] = d.getFullYear();

var expiry	= ["fldCardExpiryYear", 11];
var bkFrom	= ["slFromYear", 2];
var bkTo	= ["slToYear", 2];
var els		= [expiry, bkFrom, bkTo];

for (var i = 0; i < els.length; i++) {
	for (var j = 0; j < els[i][1]; j++) {
		var insert = "";

		y.push(y[y.length - 1] + 1);

		if (j == 0) {
			insert = "<option value=\"" + y[j] + "\" selected=\"selected\">" + y[j] + "</option>";
		} else {
			insert = "<option value=\"" + y[j] + "\">" + y[j] + "</option>";
		};
		document.getElementsByName(els[i][0])[0].innerHTML += insert;
	};
	y.length = 1; // reset y to pre-loop value
};

Refactorings

No refactoring yet !

D41d8cd98f00b204e9800998ecf8427e

lomax

September 17, 2010, September 17, 2010 21:22, permalink

2 ratings. Login to rate!

Completely untested, and without adding any sensible error checking (the assumption being that you control caller and target).

Also, i would use the DOM over constructing html strings, but that's your prerogative.

populateDropDownRanges();

function populateDropDownRanges(){
    var elts=[
        {id:"fldCardExpiryYear",range:11},
        {id:"slFromYear",range:2},
        {id:"slToYear",range:2}
    ];
    var year=new Date().getFullYear();
    for(var i=0;i<elts.length;i++){
        populateDropDownRange(document.getElementsByName(elts[i].id)[0],year,elts[i].range);
    }
}

function populateDropDownRange(elt,start,range){
    var options=[];
    for(var i=0;i<range;i++){
        options.push(['<option value="','">','</option>'].join(start+i));
    }
    elt.innerHTML=options.join('');
    elt.selectedIndex=0;
}
55502f40dc8b7c769880b10874abc9d0

garreh.myopenid.com

September 20, 2010, September 20, 2010 22:30, permalink

1 rating. Login to rate! 
var elements = {
  'fldCardExpiryYear':  { range: 11 },
  'slFromYear':         { range: 2 },
  'slToYear':           { range: 2 }
};

var getOptions = function(start, end) {
  for (var buffer = '', i = start; i <= end; i++)
  {
    buffer += '<option value="' + i + '">' + i + '</option>';
  }
  return buffer;
};

var currentYear = new Date().getFullYear(), name;

for (name in elements)
{
  var options = getOptions(currentYear, currentYear + elements[name].range);
  var element = document.getElementsByName(name)[0];
  
  if (element)
  {
    element.innerHTML = options;
    element.selectedIndex = 0;
  }
}
55502f40dc8b7c769880b10874abc9d0

garreh.myopenid.com

September 21, 2010, September 21, 2010 12:45, permalink

1 rating. Login to rate!

I do like lomax's way of using .join() to add the year values, but I think the naming between populateDropDownRange and populateDropDownRanges is clumsy. I would use something along these lines. I've encapsulated it all inside a self executing anonymous function (good practice)

(function() {

  var elements = {
      'fldCardExpiryYear':  { range: 11 },
      'slFromYear':         { range: 2 },
      'slToYear':           { range: 2 }
    },
    year = new Date().getFullYear(),
    name;

  function getOptions(start, range) {
    for (var r = [], i = 0; i < range; i++)
    {
      r.push(['<option value="', '">', '</option>'].join(start + i));
    }
    return r.join('');
  };

  for (name in elements)
  {
    var element = document.getElementsByName(name)[0];
    
    element.innerHTML     = getOptions(year, elements[name].range);
    element.selectedIndex = 0;
  }

})();


///shorted down to this once compressed:

(function(){function f(g,h){for(var c=[],a=0;a<h;a++)c.push(['<option value="','">',"</option>"].join(g+a));return c.join("")}var d={fldCardExpiryYear:{a:11},slFromYear:{a:2},slToYear:{a:2}},i=(new Date).getFullYear(),b;for(b in d){var e=document.getElementsByName(b)[0];e.innerHTML=f(i,d[b].a);e.selectedIndex=0}})();

Your refactoring





Format Copy from initial code

or Cancel