JavaScript refactoring - How can this code be refactored? [closed]

Closed. This question is off-topic. It is not currently accepting answers.

---

Want to improve this question? Update the question so it's on-topic for Stack Overflow.

Closed 11 years ago.

Improve this question

<script type="text/javascript">
              var typeSort = document.getElementById('sortTypeUp').value;
              if(typeSort == '1'){
                 document.getElementById('bewertung').className='tab_subbox tab2 tabon';
                 document.getElementById('price').className='tab_subbox tab3';
                 document.getElementById('kategorie').className='tab_subbox tab4';
                 document.getElementById('distance').className='tab_subbox tab5';
                 document.getElementById('bypois').className='tab_subbox tab6';
                 document.getElementById('opodoTriangle').style.display='block';
                 document.getElementById('priceTriangle').style.display='none';
                 document.getElementById('reviewTriangle').style.display='none';
                 document.getElementById('distanceTriangle').style.display='none';
                 document.getElementById('bypoisTriangle').style.display='none';
              }
              else if(typeSort == '2'){
                 document.getElementById('bewertung').className='tab_subbox tab2';
                 document.getElementById('price').className='tab_subbox tab3 tabon';
                 document.getElementById('kategorie').className='tab_subbox tab4';
                 document.getElementById('distance').className='tab_subbox tab5';
                 document.getElementById('bypois').className='tab_subbox tab6';
                 document.getElementById('opodoTriangle').style.display='none';
                 document.getElementById('priceTriangle').style.display='block';
                 document.getElementById('reviewTriangle').style.display='none';   
                 document.getElementById('distanceTriangle').style.display='none';
                 document.getElementById('bypoisTriangle').style.display='none';
              } 
              else if(typeSort == '3'){
                 document.getElementById('bewertung').className='tab_subbox tab2';
                 document.getElementById('price').className='tab_subbox tab3';
                 document.getElementById('kategorie').className='tab_subbox tab4 tabon';
                 document.getElementById('distance').className='tab_subbox tab5';
                 document.getElementById('bypois').className='tab_subbox tab6';
                 document.getElementById('opodoTriangle').style.display='none';
                 document.getElementById('priceTriangle').style.display='none';
           开发者_如何学运维      document.getElementById('reviewTriangle').style.display='block';      
                 document.getElementById('distanceTriangle').style.display='none';
                 document.getElementById('bypoisTriangle').style.display='none';      
              }
              else if(typeSort == '4'){
                 document.getElementById('bewertung').className='tab_subbox tab2';
                 document.getElementById('price').className='tab_subbox tab3';
                 document.getElementById('kategorie').className='tab_subbox tab4';
                 document.getElementById('distance').className='tab_subbox tab5 tabon';
                 document.getElementById('bypois').className='tab_subbox tab6';
                 document.getElementById('opodoTriangle').style.display='none';
                 document.getElementById('priceTriangle').style.display='none';
                 document.getElementById('reviewTriangle').style.display='none';      
                 document.getElementById('distanceTriangle').style.display='block';
                 document.getElementById('bypoisTriangle').style.display='none';      
              }
              else if(typeSort == '5'){
                 document.getElementById('bewertung').className='tab_subbox tab2';
                 document.getElementById('price').className='tab_subbox tab3';
                 document.getElementById('kategorie').className='tab_subbox tab4';
                 document.getElementById('distance').className='tab_subbox tab5';
                 document.getElementById('bypois').className='tab_subbox tab6 tabon';
                 document.getElementById('opodoTriangle').style.display='none';
                 document.getElementById('priceTriangle').style.display='none';
                 document.getElementById('reviewTriangle').style.display='none';      
                 document.getElementById('distanceTriangle').style.display='none';
                 document.getElementById('bypoisTriangle').style.display='block';      
              }
           </script>

---

In flexible languages like JavaScript, Python and others which provide literal notation for core data structures, I find it handy to build simple data structures which represent the parts which vary, then build the logic around the structure.

var tabs = [
  {id: 'bewertung', contentId: 'opodoTriangle'}
, {id: 'price', contentId: 'priceTriangle'}
, {id: 'kategorie', contentId: 'reviewTriangle'}
, {id: 'distance', contentId: 'distanceTriangle'}
, {id: 'bypois', contentId: 'bypoisTriangle'}
];

var activeIdx = parseInt(document.getElementById('sortTypeUp').value, 10) - 1;
for (var i = 0, l = tabs.length; i < l; i++) {
  var tab = tabs[i]
    , active = (i == activeIdx)
    , tabEl = document.getElementById(tab.id)
    , contentEl = document.getElementById(tab.contentId);
  if (active) {
    addClass(tabEl, 'tabon');
  } else {
    removeClass(tabEl, 'tabon');
  }
  contentEl.style.display = (active ? 'block' : 'none');
}

Class management utility methods (from http://dean.edwards.name/IE7/caveats/ - just grabbed the first implementation I found):

function addClass(element, className) {
  if (!hasClass(element, className)) {
    if (element.className) element.className += ' ' + className;
    else element.className = className;
  }
};

function removeClass(element, className) {
  var regexp = new RegExp('(^|\\s)' + className + '(\\s|$)');
  element.className = element.className.replace(regexp, '$2');
};

function hasClass(element, className) {
  var regexp = new RegExp('(^|\\s)' + className + '(\\s|$)');
  return regexp.test(element.className);
};

Bonus: here's a brevity-for-brevity's-sake version which makes use of type coercion, the comma operator, for loop assingment and exit logic and functions as first class objects - for example only! :)

var activeIdx = +document.getElementById('sortTypeUp').value - 1;
for (var i = 0, tab, active; active = (i == activeIdx), tab = tabs[i]; i++) {
  (active ? addClass : removeClass)(document.getElementById(tab.id), 'tabon');
  document.getElementById(tab.contentId).style.display =
      (active ? 'block' : 'none');
}

---

    <script type="text/javascript">

       var typeSort = document.getElementById('sortTypeUp').value;

       document.getElementById('bewertung').className = 'tab_subbox tab2' + ((typeSort=='1') ? ' tabon' : '');
       document.getElementById('price').className='tab_subbox tab3' + ((typeSort=='2') ? ' tabon' : '');
       document.getElementById('kategorie').className='tab_subbox tab4' + ((typeSort=='3') ? ' tabon' : '');
       document.getElementById('distance').className='tab_subbox tab5' + ((typeSort=='4') ? ' tabon' : '');
       document.getElementById('bypois').className='tab_subbox tab6' + ((typeSort=='5') ? ' tabon' : '');
       document.getElementById('opodoTriangle').style.display = ((typeSort=='1') ? 'block' : 'none');
       document.getElementById('priceTriangle').style.display=((typeSort=='2') ? 'block' : 'none');
       document.getElementById('reviewTriangle').style.display=((typeSort=='3') ? 'block' : 'none');
       document.getElementById('distanceTriangle').style.display=((typeSort=='4') ? 'block' : 'none');
       document.getElementById('bypoisTriangle').style.display=((typeSort=='5') ? 'block' : 'none');

    </script>

---

You can put the common part of the code in functions, like this:

function active(id, tab, typeSort, eq){
  document.getElementById(id).className='tab_subbox '+tab+(typeSort==eq?' tabon':'');
}

function show(id, typeSort, eq){
  document.getElementById(id).style.display=typeSort==eq?'block':'none';
}

var typeSort = parseInt(document.getElementById('sortTypeUp').value);

active('bewertung', 'tab2', typeSort, 1);
active('price', 'tab3', typeSort, 2);
active('kategorie', 'tab4', typeSort, 3);
active('distance', 'tab5', typeSort, 4);
active('bypois', 'tab6', typeSort, 5);

show('opodoTriangle', typeSort, 1);
show('priceTriangle', typeSort, 2);
show('reviewTriangle', typeSort, 3);      
show('distanceTriangle', typeSort, 4);
show('bypoisTriangle', typeSort, 5);

---

Maybe create a method which takes two arrays as parameters: One array contains the ids of the elements whose class should be reset and the other array contains the corresponding new styles.

function updateClasses(ids, classes) {
// implement me
}

Init the element array just once:

var ids=new Array("bewertung","price","kategorie", ...);

And for every case:

if(typeSort == '1'){
    var classes=new Array("tab_subbox tab2 tabon","tab_subbox tab3","tab_subbox tab4", ...);
    updateClasses(ids, classes);
} else if (typeSort == '2') {
    var classes = // some other values
    updateClasses(ids, classes);
} // ...

Edit

Also use a switch statement, like John said.

---

Place the common code at top that includes css class assignment e.g.

document.getElementById('bewertung').className='tab_subbox tab2';

and more likely as you are marking your objects hidden that appears to be a common code too... place it at top and only mark those objects visible which are required

use case statements instead of Ifs and also use Jquery that will improve your code style.

---

var get = document.getElementById;
var elems = ['bewertung', 'price', 'kategorie', 'distance', 'bypois'];
var triangles = ['opodoTriangle', 'priceTriangle', 'reviewTriangle', 'distanceTriangle', 'bypoisTriangle'];
var typeSort = +get('sortTypeUp').value - 1;

elems.forEach(function(v) {
    var elem = get(v);
    elem.classList.remove('tabon');
});

triangles.forEach(function(v) {
    var elem = get(v);
    elem.style.display = 'none';
});

elems[typeSort].classList.add('tabon');
triangle[typeSort].style.display = 'block';

Using DOM3 / ES5.

Use arrays and treat typeSort as an array index.

.classList shim, ES5 shim

---

typeSort = parseInt( document.getElementById('sortTypeUp').value );

s = ["bewertung", "price", "kategorie", "distance", "bypois"];
for(i=0;i<s.length;i++) {
  e = document.getElementById(s[i]);
  n = i + 2;
  e.className = "tab_subbox tab" + n;
  if(typeSort == (i+1)) {
     e.className = e.className + " tabon";
  }
}

s = ["opodo", "price", "review", "distance", "bypois"];
for(i=0;i<s.length;i++) {
  e = document.getElementById( s[i] + "Triangle" );
  e.style.display = 'none';
  if(typeSort == (i+1)) {
    e.style.display = 'block';
  }
}