//e is the object, and 'op' is a parameter passed that tells us
//if we're increasing or decreasing the value.
function step_change(e, op) {
var changeVal;
//If the value contains a period, then it should be a
//small change. The value is split so that the script
//knows how many decimal places to set the value to
//once the value is changed.
if ($(e).value.match(/[.]/g)) {
var numVal = $(e).value.split(".");
if (numVal[1].length > 2)
changeVal = 0.001;
else
changeVal = 0.05;
}
else if (parseFloat($(e).value / 10) <= 100)
changeVal = 5;
else if (parseFloat($(e).value / 10) >= 1000)
changeVal = 500;
else if (parseFloat($(e).value / 10) <= 1000)
changeVal = 50;
else
changeVal = 5;
//altKeyMod is set elsewhere in the app and doubles the
//amount if the alt key is held down.
if (altKeyMod)
changeVal = changeVal * 2;
if (op == "+")
$(e).value = parseFloat($(e).value) + changeVal;
else
$(e).value = parseFloat($(e).value) - changeVal;
if(numVal)
$(e).value = parseFloat($(e).value).toFixed(numVal[1].length);
}
Refactorings
No refactoring yet !
paul.wilkins.myopenid.com
June 14, 2009, June 14, 2009 00:08, permalink
Some good ways to refactor this is to remove the math calculations.
Instead of checking if value/10 <= 100, check that value <= 1000
Then you can use an array that matches up the value limit with the step value.
Or, you can avoid all of that and make the step value 2% of the next multiple of 10.
musicfreak
July 2, 2009, July 02, 2009 21:15, permalink
Some things:
1) You don't need to make the same calculations every time. For example, you have parseFloat($(e).value) six times in your code. You could save that as a variable and just reuse it.
2) In your first conditional, you don't need that last "else". It will never fire.
3) Instead of doing floatVal / 10 every time, I just multiplied the right-hand number by 10 to save you some math.
4) Using a regexp to just check to see if the string has a period is a little overboard. I replaced it with indexOf(), which is faster. (Although it really doesn't matter in this example, but I wanted to make a point.)
5) changeVal = changeVal * 2 can be shortened to changeVal *= 2.
Also some other little nitpicky things, but you get the idea. :)
//e is the object, and 'op' is a parameter passed that tells us
//if we're increasing or decreasing the value.
function step_change(e, op) {
var changeVal = 0;
var value = $(e).value;
var floatVal = parseFloat(value);
//If the value contains a period, then it should be a
//small change. The value is split so that the script
//knows how many decimal places to set the value to
//once the value is changed.
if (value.indexOf(".") > -1) {
var numVal = $(e).value.split(".");
if (numVal[1].length > 2)
changeVal = 0.001;
else
changeVal = 0.05;
}
else if (floatVal <= 1000)
changeVal = 5;
else if (floatVal <= 10000)
changeVal = 50;
else
changeVal = 500;
//altKeyMod is set elsewhere in the app and doubles the
//amount if the alt key is held down.
if (altKeyMod)
changeVal *= 2;
if (op == "+")
floatVal += changeVal;
else
floatVal -= changeVal;
if (numVal)
floatVal = floatVal.toFixed(numVal[1].length);
$(e).value = floatVal;
}
Essentially there is a text box with arrows on either side that change the value up and down. But it is smart because the step is different depending on the size of the value in the box.
A more concise/cleaner way to do this would be appreciated.