0
\$\begingroup\$

I have the following javascript that switches the different views. All works fine however it is extremely repetitive.

I would like to clean it up to be optimally usable, but not to sure how I would go about it.

Any help with this would be highly appreciated.

Below is my code that i wrote in order to get it to work. It works well but seems very bloated... don't know what more to say about it but red box telling me to add more text is just not going away, so not to sure how much more i am suppose to type in order to get some assistance with my question at had here.

function switchView () {
var viewContainer = document.getElementById('content-wrapper');
var tileBtn = document.getElementById('tile-btn');
var listBtn = document.getElementById('list-btn');
var swipeBtn = document.getElementById('swipe-btn');
tileBtn.addEventListener( 'click', function(e){
if (viewContainer.classList.contains('list-view') || 
 viewContainer.classList.contains('swipe-view') ) {
 e.preventDefault();
 viewContainer.classList.remove('list-view');
 viewContainer.classList.remove('swipe-view');
 viewContainer.classList.add('tile-view');
}
});
listBtn.addEventListener( 'click', function(e){
if (viewContainer.classList.contains('tile-view') || 
 viewContainer.classList.contains('swipe-view') ) {
 e.preventDefault();
 viewContainer.classList.remove('tile-view');
 viewContainer.classList.remove('swipe-view');
 viewContainer.classList.add('list-view');
}
});
swipeBtn.addEventListener( 'click', function(e){
if (viewContainer.classList.contains('list-view') || 
viewContainer.classList.contains('tile-view') ) {
 e.preventDefault();
 viewContainer.classList.remove('list-view');
 viewContainer.classList.remove('tile-view');
 viewContainer.classList.add('swipe-view');
}
});
};
switchView();
t3chb0t
44.6k9 gold badges84 silver badges190 bronze badges
asked Jul 18, 2017 at 13:31
\$\endgroup\$
2
  • 1
    \$\begingroup\$ It would be easier to help you if you provided the HTML on which this code acts. There are likely changes you could make there that would make this code less repetitive. \$\endgroup\$ Commented Jul 18, 2017 at 14:57
  • \$\begingroup\$ You are probably doing too much in JavaScript. It's hard to advise you properly, though, given the lack of context and detail in this question. \$\endgroup\$ Commented Sep 16, 2017 at 17:36

2 Answers 2

2
\$\begingroup\$

Generally, when you have a lot of duplicated code you should try to take out the parts that are different and put them in an array, then loop the values and perform the operation based on the values.

Here's a quick re-factor I threw together of your code with that in mind. It could probably be improved upon using JS6 stuff like let instead of the closure but at least this should illustrate the point.

function switchView () {
 var viewContainer = document.getElementById('content-wrapper');
 var buttons = [
 {id: 'tile-btn', reqdClasses: ['list-view', 'swipe-view'], newClass: 'tile-view'},
 {id: 'list-btn', reqdClasses: ['tile-view', 'swipe-view'], newClass: 'list-view'},
 {id: 'swipe-btn', reqdClasses: ['list-view', 'tile-view'], newClass: 'swipe-view'}
 ];
 for(var i=buttons.length; i--;)(function(i){
 var button = document.getElementById(buttons[i].id);
 button.addEventListener('click', function(e){
 for(var n = buttons[i].reqdClasses.length; n--;){
 if(!button.classList.contains(buttons[i].reqdClasses[n])) continue;
 e.preventDefault();
 for(var n = buttons[i].reqdClasses.length; n--;) button.classList.remove(buttons[i].reqdClasses[n]);
 button.classList.add(buttons[i].newClass);
 break;
 }
 });
 })(i);
}
answered Jul 18, 2017 at 15:06
\$\endgroup\$
1
\$\begingroup\$

In these kinds of situations, it's often easier to just remove all of the classes, then add back the one you want. This generally happens fast enough that the user doesn't notice any flashing.

function viewSwitch(which) {
 viewContainer.classList.remove('list-view','swipe-view','tile-view');
 viewContainer.add(which);
}
function switchView() {
 var viewContainer = document.getElementById('content-wrapper');
 var tileBtn = document.getElementById('tile-btn');
 var listBtn = document.getElementById('list-btn');
 var swipeBtn = document.getElementById('swipe-btn');
 tileBtn.addEventListener('click', function(e) {
 e.preventDefault();
 viewSwitch('tile-view');
 });
 listBtn.addEventListener('click', function(e) {
 e.preventDefault();
 viewSwitch('list-view');
 });
 swipeBtn.addEventListener('click', function(e) {
 e.preventDefault();
 viewSwitch('swipe-view');
 });
};
switchView();
answered Jul 18, 2017 at 15:26
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.