I think i put the anonymous function in there wrong... when it outputs listzonebuffs it includes the function(){... part.

function load(zone){

setupzonebuffs(zone);

document.getElementById('zonetitle').innerHTML=zone;

listzonebuffs="";
if(zonebuffs['B']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["B"]>1){return "good";}else{return "bad";}}+"\'>Brute Force "+function(){if(zonebuffs['B']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['B'])*100))+"%</span>";}
if(zonebuffs['W']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["W"]>1){return "good";}else{return "bad";}}+"\'>Wind "+function(){if(zonebuffs['W']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['W'])*100))+"%</span>";}
if(zonebuffs['I']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["I"]>1){return "good";}else{return "bad";}}+"\'>Ice "+function(){if(zonebuffs['I']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['I'])*100))+"%</span>";}
if(zonebuffs['E']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["E"]>1){return "good";}else{return "bad";}}+"\'>Energy "+function(){if(zonebuffs['E']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['E'])*100))+"%</span>";}
if(zonebuffs['F']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["F"]>1){return "good";}else{return "bad";}}+"\'>Fire "+function(){if(zonebuffs['F']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['F'])*100))+"%</span>";}
if(zonebuffs['R']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["R"]>1){return "good";}else{return "bad";}}+"\'>Earth "+function(){if(zonebuffs['R']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['R'])*100))+"%</span>";}
if(zonebuffs['A']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["A"]>1){return "good";}else{return "bad";}}+"\'>Astronomical "+function(){if(zonebuffs['A']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['A'])*100))+"%</span>";}
if(zonebuffs['S']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["S"]>1){return "good";}else{return "bad";}}+"\'>Stealth "+function(){if(zonebuffs['S']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['S'])*100))+"%</span>";}
document.getElementById('listzonebuffs').innerHTML=listzonebuffs;

}

var zonebuffs=new Array();
zonebuffs['B']=1;zonebuffs['W']=1;zonebuffs['I']=1;zonebuffs['E']=1;zonebuffs['F']=1;zonebuffs['R']=1;zonebuffs['A']=1;zonebuffs['S']=1;

function setupzonebuffs(zone){
switch(zone){
case 'Mist':
zonebuffs['S']=1.3;zonebuffs['W']=1.1;zonebuffs['F']=.9;
break;
}
}

I think i put the anonymous function in there wrong... when it outputs listzonebuffs it includes the function(){... part.

function load(zone){

setupzonebuffs(zone);

document.getElementById('zonetitle').innerHTML=zone;

listzonebuffs="";
if(zonebuffs['B']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["B"]>1){return "good";}else{return "bad";}}+"\'>Brute Force "+function(){if(zonebuffs['B']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['B'])*100))+"%</span>";}
if(zonebuffs['W']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["W"]>1){return "good";}else{return "bad";}}+"\'>Wind "+function(){if(zonebuffs['W']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['W'])*100))+"%</span>";}
if(zonebuffs['I']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["I"]>1){return "good";}else{return "bad";}}+"\'>Ice "+function(){if(zonebuffs['I']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['I'])*100))+"%</span>";}
if(zonebuffs['E']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["E"]>1){return "good";}else{return "bad";}}+"\'>Energy "+function(){if(zonebuffs['E']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['E'])*100))+"%</span>";}
if(zonebuffs['F']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["F"]>1){return "good";}else{return "bad";}}+"\'>Fire "+function(){if(zonebuffs['F']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['F'])*100))+"%</span>";}
if(zonebuffs['R']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["R"]>1){return "good";}else{return "bad";}}+"\'>Earth "+function(){if(zonebuffs['R']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['R'])*100))+"%</span>";}
if(zonebuffs['A']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["A"]>1){return "good";}else{return "bad";}}+"\'>Astronomical "+function(){if(zonebuffs['A']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['A'])*100))+"%</span>";}
if(zonebuffs['S']!=1){listzonebuffs+="<span class=\'"+function(){if(zonebuffs["S"]>1){return "good";}else{return "bad";}}+"\'>Stealth "+function(){if(zonebuffs['S']>1){return "+";}else{return "-";}}+" "+Math.round(Math.abs((1-zonebuffs['S'])*100))+"%</span>";}
document.getElementById('listzonebuffs').innerHTML=listzonebuffs;

}

var zonebuffs=new Array();
zonebuffs['B']=1;zonebuffs['W']=1;zonebuffs['I']=1;zonebuffs['E']=1;zonebuffs['F']=1;zonebuffs['R']=1;zonebuffs['A']=1;zonebuffs['S']=1;

function setupzonebuffs(zone){
switch(zone){
case 'Mist':
zonebuffs['S']=1.3;zonebuffs['W']=1.1;zonebuffs['F']=.9;
break;
}
}

• javascript

• 10 that's just terrible. – pstanton Commented Nov 10, 2009 at 23:21
• 2 A few loops would do that code wonders. – Nosredna Commented Nov 10, 2009 at 23:26
• 4 The backspace key would do that code wonders. – aehiilrs Commented Nov 10, 2009 at 23:32
• 3 Come on, give the guy a break. I agree it could be better, let's be more forgiving :) – o.k.w Commented Nov 10, 2009 at 23:37
• 3 I agree, o.k.w. We should be nice. But he's having trouble with it for the same reason we are--it's copypasta. – Nosredna Commented Nov 10, 2009 at 23:38

6 Answers 6

You should cleanup this code (honestly it's unreadable) and try replacing: +function(){if(COND){A}{B}}+ by +(COND ? A : B)+

That's because probably instead of appending the function object to the string, you wanted to append the function call result, that is, instead of

x += "<span ...>" + function() { ... } + ...

you probably wanted

x += "<span ...>" + (function() { ... })() + ...

However, I must say that your question isn't very obvious. Next time try providing a smaller sample with the localized problem and explain it more clearly. While doing that, most of the times, people end up figuring out the solution themselves...

Mauris was on the right track, but there's still a "DRY" violation at work here. That contributes a lot to the illegibility of the code. Also makes it a lot harder to figure out what's wrong.

If you could resist the urge to cut and paste so much, and do a bit more thinking up front, you can save yourself a lot of agony and enjoy the process a lot more.

Here's my take on the code:

function goodOrBad (key){ 
    if(zonebuffs[key] > 1){
       return "good";
    } else {
       return "bad";
    }
}


function percentageFor (key){
  return (zonebuffs[key] > 1 ? "+" : "-") + " " + Math.round(Math.abs((1-zonebuffs['W'])*100))+"%";
}

function textForZoneBuff(key, zone) {
  if(zonebuffs[key]!=1){ 
    return "<span class='"+ goodOrBad(key) + "'>" + zone + " " + percentageFor(key) + "</span> ";
  } else {
    return "";
  }
}

function load(zone){

  setupzonebuffs(zone);
  document.getElementById('zonetitle').innerHTML=zone;

  listzonebuffs= textForZoneBuff('B','Brute Force') 
               + textForZoneBuff('W','Wind') 
               + textForZoneBuff('I','Ice') 
               + textForZoneBuff('E','Energy') 
               + textForZoneBuff('F','Fire') 
               + textForZoneBuff('R','Earth') 
               + textForZoneBuff('A','Astronomical') 
               + textForZoneBuff('S','Stealth');

  document.getElementById('listzonebuffs').innerHTML=listzonebuffs;
}


var zonebuffs=new Array();

zonebuffs['B']=1;zonebuffs['W']=1;zonebuffs['I']=1;zonebuffs['E']=1;zonebuffs['F']=1;zonebuffs['R']=1;zonebuffs['A']=1;zonebuffs['S']=1;

function setupzonebuffs(zone){
switch(zone){
case 'Mist':
zonebuffs['S']=1.3;zonebuffs['W']=1.1;zonebuffs['F']=.9;
break;
}
}

Ahhh... now I can see straight again :-)

Use the ternary operator instead of function

When using anonymous functions, the value returned is a function.

Example:

var func_a = function(){
  alert('testing');
}
func_a();

In your case what you needed is to use ternary operator, which conditionally returns the value based on the expression.

CONDITION ? TRUE : FALSE, where based on CONDITION, value in TRUE or FALSE will be returned.

Your code, fixed:

function load(zone){

setupzonebuffs(zone);

document.getElementById('zonetitle').innerHTML=zone;

listzonebuffs="";
if(zonebuffs['B']!=1){listzonebuffs+="<span class=\'"+(zonebuffs["B"]>1?"good":"bad")+"\'>Brute Force "+(zonebuffs['B']>1?"+":"-")+" "+Math.round(Math.abs((1-zonebuffs['B'])*100))+"%</span>";}
if(zonebuffs['W']!=1){listzonebuffs+="<span class=\'"+(zonebuffs["W"]>1?"good":"bad")+"\'>Wind "+(zonebuffs['W']>1?"+":"-")+" "+Math.round(Math.abs((1-zonebuffs['W'])*100))+"%</span>";}
if(zonebuffs['I']!=1){listzonebuffs+="<span class=\'"+(zonebuffs["I"]>1?"good":"bad")+"\'>Ice "+(zonebuffs['I']>1?"+":"-")+" "+Math.round(Math.abs((1-zonebuffs['I'])*100))+"%</span>";}
if(zonebuffs['E']!=1){listzonebuffs+="<span class=\'"+(zonebuffs["E"]>1?"good":"bad")+"\'>Energy "+(zonebuffs['E']>1?"+":"-")+" "+Math.round(Math.abs((1-zonebuffs['E'])*100))+"%</span>";}
if(zonebuffs['F']!=1){listzonebuffs+="<span class=\'"+(zonebuffs["F"]>1?"good":"bad")+"\'>Fire "+(zonebuffs['F']>1?"+":"-")+" "+Math.round(Math.abs((1-zonebuffs['F'])*100))+"%</span>";}
if(zonebuffs['R']!=1){listzonebuffs+="<span class=\'"+(zonebuffs["R"]>1?"good":"bad")+"\'>Earth "+(zonebuffs['R']>1?"+":"-")+" "+Math.round(Math.abs((1-zonebuffs['R'])*100))+"%</span>";}
if(zonebuffs['A']!=1){listzonebuffs+="<span class=\'"+(zonebuffs["A"]>1?"good":"bad")+"\'>Astronomical "+(zonebuffs['A']>1?"+":"-")+" "+Math.round(Math.abs((1-zonebuffs['A'])*100))+"%</span>";}
if(zonebuffs['S']!=1){listzonebuffs+="<span class=\'"+(zonebuffs["S"]>1?"good":"bad")+"\'>Stealth "+(zonebuffs['S']>1?"+":"-")+" "+Math.round(Math.abs((1-zonebuffs['S'])*100))+"%</span>";}
document.getElementById('listzonebuffs').innerHTML=listzonebuffs;

}

var zonebuffs=new Array();
zonebuffs['B']=1;zonebuffs['W']=1;zonebuffs['I']=1;zonebuffs['E']=1;zonebuffs['F']=1;zonebuffs['R']=1;zonebuffs['A']=1;zonebuffs['S']=1;

function setupzonebuffs(zone){
switch(zone){
case 'Mist':
zonebuffs['S']=1.3;zonebuffs['W']=1.1;zonebuffs['F']=.9;
break;
}
}

Please do the formatting...

And to reduce redundancy:

function cond1(val){
  return (val>1?"good":"bad");
}

function cond2(val){
  return (val>1?"+":"-");
}
function val1(val){
  return Math.round(Math.abs((1-val)*100));
}
if(zonebuffs['A']!=1){listzonebuffs+="<span class=\'"+cond1(zonebuffs["A"])+"\'>Astronomical "+cond2(zonebuffs['A'])+" "+val1(zonebuffs['A'])+"%</span>";}
if(zonebuffs['S']!=1){listzonebuffs+="<span class=\'"+cond1(zonebuffs["S"])+"\'>Stealth "+cond2(zonebuffs['S'])+" "+val1(zonebuffs['S'])+"%</span>";}

You only create a function, whilst you shall call it - adding () after closing function definition solves the problem. But the simplier solution is to use

if(zonebuffs['W']!=1){listzonebuffs+="1? "good" : "bad") +"\'>Wind "+ (zonebuffs['W']>1 ? "+": "-")+" "+Math.round(Math.abs((1-zonebuffs['W'])*100))+"%";}

this might be a good application for a function within a function. You might try something like this:

function load(zone){
        setupzonebuffs(zone);
        document.getElementById('zonetitle').innerHTML=zone;

        var oneCompare = function(val, onTrue, onFalse){
            return (val > 1) ? onTrue : onFalse;
        };

        listzonebuffs="";
        if(zonebuffs['B']!=1){listzonebuffs+="<span class=\'"+oneCompare(zonebuffs["B"],'good','bad')+"\'>Brute Force "+oneCompare(zonebuffs["B"],'+','-')+" "+Math.round(Math.abs((1-zonebuffs['B'])*100))+"%</span>";}
        //More...

}