JavaScript refactoring - How can this code be refactored?

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>

Can you please give a little bit more of an explanation instead of just dumping code into a question? — John, Jun 10 '11 at 08:51
This question should be moved to http://codereview.stackexchange.com/ — Lekensteyn, Jun 10 '11 at 08:53
possible duplication [How do you refactor JavaScript, HTML, CSS, etc?](http://stackoverflow.com/questions/1446267/how-do-you-refactor-javascript-html-css-etc) — eros, Jun 10 '11 at 08:54

Jonny Buchanan · Answer 1 · 2011-06-10T09:46:49.997

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');
}

The code is far too terse. Too much magic in the for loop. and generally hard to read. — Raynos, Jun 10 '11 at 09:28
+1 that reads a lot better. The brevity version is something the closure compiler should make for you ;) — Raynos, Jun 10 '11 at 10:02

score 2 · Answer 2 · answered Jun 10 '11 at 08:59

    <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>

The readability and maintainability do suffer, but as Raynos said, it is terser. — deltaforce2, Jun 10 '11 at 09:11

score 2 · Answer 3 · answered Jun 10 '11 at 09:13

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);

An improvement on @Sascha answer. I'd be nice if arrays were used instead of ternaries. — Raynos, Jun 10 '11 at 09:18

score 0 · Answer 4 · answered Jun 10 '11 at 08:56

0

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.

answered Jun 10 '11 at 08:56

deltaforce2

583
2
8

@Raynos Could you specify what is wrong with it? I know I'm not so fluent with JS so I picked up an example array initialization from w3schools (http://w3schools.com/js/js_obj_array.asp). Plus this is supposed to be an example to describe my ideas. The actual implementation should be done by the OP. – deltaforce2 Jun 10 '11 at 09:06
It's not actaully wrong, sorry about that. I'd also recommend you reference [mdc](https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array) instead of W3Schools as w3schools has a habit of getting it wrong aswell. – Raynos Jun 10 '11 at 09:08
http://w3fools.com/ and the preferred array construction is using `["bewertung", "price" /* etc */]` – Lekensteyn Jun 10 '11 at 09:08
@Raynos OK thanks for that. W3Schools just happened to be the first Google hit on this subject, so I picked that one. – deltaforce2 Jun 10 '11 at 09:09

score 0 · Answer 5 · answered Jun 10 '11 at 08:57

0

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.

answered Jun 10 '11 at 08:57

Usman Masood

1,937
2
17
33

1

jQuery does not improve your code style. – Raynos Jun 10 '11 at 09:14

Raynos · Answer 6 · 2011-06-10T09:13:23.327

0

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

edited Jun 10 '11 at 09:13

answered Jun 10 '11 at 09:00

Raynos

166,823
56
351
396

Looking at the class names and ID's, I doubt if the visitor uses such a modern browser :D – Lekensteyn Jun 10 '11 at 09:11
@Lekensteyn just add ES5 shim & class list shim and it should fix it up – Raynos Jun 10 '11 at 09:12

gsbabil · Answer 7 · 2011-06-10T09:54:21.327

0

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';
  }
}

edited Jun 10 '11 at 09:54

answered Jun 10 '11 at 09:15

gsbabil

7,505
3
26
28

Is not the same as the above code. – Raynos Jun 10 '11 at 09:26

JavaScript refactoring - How can this code be refactored?

7 Answers7