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 2 down vote favorite

I have a function called FormatCurrency() which I would like to call on each of the following elements:

 TextBoxA
 TextBoxB
 TextBoxC
 ...
 TextBoxN

My current function performs the following operation

 function formatALL()
 {
      //get the value into var v. call FormatCurrency(v) and store it back
      var v = document.getElementById("TextBoxA_TextBox").value;
  document.getElementById("TextBoxA_TextBox").value = FormatCurrency(v);

      v = document.getElementById("TextBoxB_TextBox").value;
  document.getElementById("TextBoxB_TextBox").value = FormatCurrency(v);        

  v = document.getElementById("TextBoxC_TextBox").value;
  document.getElementById("TextBoxC_TextBox").value = FormatCurrency(v); 

      //etc... all the way til my last TextBox
 }

Is it possible to optimize this code? In other words, does anyone know any slick loops/operations/etc. to perform this same task?

Thanks!

share|improve this question

migrated from stackoverflow.com Jul 10 '12 at 14:02

This question came from our site for professional and enthusiast programmers.
3  
are you familiar with jQuery? –  c0deNinja Jul 5 '12 at 23:59
1  
yes, jQuery would make this infinitely slicker. –  Lee Taylor Jul 6 '12 at 0:06

5 Answers 5

up vote 6 down vote accepted

You can loop through the elements using a for loop:

for(var i = 65; i <= 78; i++) {
    var box = document.getElementById("TextBox" + String.fromCharCode(i) + "_TextBox");
    box.value = FormatCurrency(box.value); 
}
share|improve this answer
    
Store the fetch into a variable and you're golden. –  TheZ Jul 6 '12 at 0:01
    
This is JavaScript, nothing with a char type and a ++ operator on it... –  Bergi Jul 6 '12 at 0:04
    
@Bergi: Ah good catch. Been jumping around too many languages lately. –  tskuzzy Jul 6 '12 at 0:06
    
Well I enumerated them with Alphabets since in actuality, they actually don't have any pattern to their names. I should've been more clear in that regards. Let's say that the names are Jon, steve, mike, etc... Still this is a good suggestion –  Robert Hsu Jul 6 '12 at 4:25
up vote 2 down vote

You could use a library such as jQuery, which allows you, for example, to select elemts using CSS selectors. In this case you could assign all your elements a class and use:

$('.currency-textbox').each(function() {
   $(this).val(FormatCurrency($(this).val()));
});
share|improve this answer
up vote 1 down vote

stick all the strings into an array and iterate over them?

something like

var myElems=["TextBoxA_TextBox","TextBoxB_TextBox","TextBoxC_TextBox"];
for(var i = 0; i < myElems.list; i++) {
    var v = document.getElementById(i).value;
    document.getElementById(i).value = FormatCurrency(v);
}
share|improve this answer
up vote 1 down vote

The quickest modification would be to store your elements in a variable:

var obj = document.getElementById("TextBoxA_TextBox");
obj.value = FormatCurrency(obj.value);

The next step would be to create a loop that repeats the process dynamically and not statically as it is currently configured.

share|improve this answer
up vote 1 down vote

Two strategies: loops and variables.

function formatALL() {
    // "A".charCodeAt(0) = 65
    // "N".charCodeAt(0) = 78
    for (var i=65; i<79; i++) {
        var chr = String.fromCharCode(i);
        var el = document.getElementById("TextBox"+chr+"_TextBox");
        el.value = FormatCurrency(el.value);
    }
}

It would be easier if you had enumerated your elements with decimal numbers, not alphanumerical characters. If the element ids grew more complicated (less predictable), you'd need to put them in an array and loop over them:

function formatALL() {
    var ids = ["Jon", "steve", "mike", ...];
    for (var i=0; i<ids.length; i++) {
        var el = document.getElementById(ids[i]+"_TextBox");
        el.value = FormatCurrency(el.value);
    }
}

An even better strategy would be to use a class instead of numbered ids (the elements don't seem to be such unique :-)), like "currency" in your case. Then you don't need to adjust the array every time you change something in the page. With a class name, you can use something like getElementsByClassName("currency") (or a library equivalent like jQuery(".currency"), then iterate over the returned NodeList.

share|improve this answer
    
In terms of optimization, I would eliminate the unnecessary container variables. –  Alex W Jul 6 '12 at 0:09
    
What do you mean with "container variable"? The only unnecessary var is "chr", which I've used to improve comprehensibility and readability. Nothing to micro-optimize here. –  Bergi Jul 6 '12 at 0:13
    
He asked what he could do to optimize the code. Even if it's "micro-optimization," creating unneccessary vars is using unnecessary memory. –  Alex W Jul 6 '12 at 0:16
    
The OP wanted to optimize code, not performance. As you can see in @tskuzzy's answer, the line gets too long for SO - that's not "good" code. Depending on your JS knowledge, you might write less code less readable - I might have done the same if it wasn't a answer on a newbies question. –  Bergi Jul 6 '12 at 0:27
    
Well I enumerated them with Alphabets since in actuality, they actually don't have any pattern to their names. I should've been more clear in that regards. Let's say that the names are Jon, steve, mike, etc... –  Robert Hsu Jul 6 '12 at 4:20

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.