Reputation: 2068

How can I optimize this snippet?

Being only able to glue pieces together to get some JS working, I came up with the following code.

if (document.getElementById("component_projector_askforproduct")){
  document.getElementById("component_projector_askforproduct").style.display = "none";
}
if (document.getElementById("askforproduct_58676")){
  document.getElementById("askforproduct_58676").className = "";
}
if (document.getElementById("longdescription_58676")){
  document.getElementById("longdescription_58676").className = "";
}

if (document.getElementById("opinions_58676")){
  document.getElementById("opinions_58676").className = "activTab";
}
if (document.getElementById("component_projector_opinions")){
  document.getElementById("component_projector_opinions").style.display = "block";
}
if (document.getElementById("component_projector_opinions_add")){
  document.getElementById("component_projector_opinions_add").style.display = "block";
}

It works, but I know it's a mess. How could I optimize and slim this code down?

Upvotes: 1

Answers (6)

Maciej Sikora

Reputation: 20162

After analyse question code I understand it is some kind of menu or tabs with possibility to activate,deactivate, show, hide elements. I created structure for this purposes with builded in methods. It can be used on many elements, it can be use on one element many times, it has also chain methods so it is possible to run many methods in one code line.

Check code of this structure and examples, if there is some questions or something is not clear then ask. I am not using ES6 or jQuery because this question is not about that.

var ME = function(){ 
  //structure represent single menu element  
  var MenuElement = function(id){

    this.el = document.getElementById(id);
  
  };

  MenuElement.prototype.hide = function(){
  
    if (this.el)
    this.el.style.display = "none";
  
    return this;
  };

  MenuElement.prototype.show = function(){

    if (this.el)
    this.el.style.display = "block";
  
    return this;
  };

  MenuElement.prototype.active = function(){

    if (this.el)
    this.el.className = "activeTab";
  
    return this;
  
  };

  MenuElement.prototype.deActive = function(){

    if (this.el)
    this.el.className = "";
  
    return this;
  };
  
  //return only obj with create method to use it without new keyword
  return {
    create: function(id){      
      return new MenuElement(id);
    }
  };
  
}();

//USE EXAMPLE - the same like in question snippet
ME.create("component_projector_askforproduct").hide();
ME.create("askforproduct_58676").deActive();
ME.create("longdescription_58676").deActive();
ME.create("opinions_58676").active();
ME.create("component_projector_opinions").show();
ME.create("component_projector_opinions_add").show();

//USE EXAMPLE MANY TIMES ON ONE ELEMENT - ONLY EXAMPLE PURPOSES
var element = ME.create("component_projector_askforproduct");
var toggle = true;
setInterval(function(){
   
   if (toggle)
   element.show().active(); //chaining example
   else
     element.deActive().hide();
  
   toggle = !toggle;
  
},1000);

div.activeTab {
     font-weight: bold;
     color: red;
 }

<div id="component_projector_askforproduct">
component_projector_askforproduct
</div>
<div id="askforproduct_58676">
askforproduct_58676
</div>
<div id="longdescription_58676">
longdescription_58676
</div>
<div id="opinions_58676">
opinions_58676
</div>
<div id="component_projector_opinions">
component_projector_opinions
</div>
<div id="component_projector_opinions_add">
component_projector_opinions_add
</div>

Upvotes: 0

A.D

Reputation: 2270

EDIT I removed my last answer, and got an idea from Mirko Vukušićs answer.

if you create css classes for hiding and showing elements you could do it like this.

css

.hide { display: none!important; }
.show { display: block!important; }

javascript

var arr = [
  ["component_projector_askforproduct", "hide"],
  ["askforproduct_58676", ""],
  ["longdescription_58676", ""],
  ["opinions_58676", "activTab"],
  ["component_projector_opinions", "show"],
  ["component_projector_opinions_add", "show"]
]

for (i = 0; i < arr.length -1; i++) {
  var elm = document.getElementById(arr[i][0]);
  if(elm) {
    elm.className = arr[i][1];
  }
}

Upvotes: 1

sinisake

Reputation: 11338

To avoid constant checking, and avoid errors, you can do this neat trick, to set default values:

function $(elem) {

return document.getElementById(elem) || document.createElement("div");
}

So, this way, if element doesn't exist, it will be created, but not appended to body!

Test:

function $(elem) {

return document.getElementById(elem) || document.createElement("div");
}

$("component_projector_askforproduct").style.display = "none";
$("askforproduct_58676").className='';
$('longdescription_58676').className="";
$('opinions_58676').className="activeTab";
$("component_projector_opinions").style.display = "block";
//etc, etc....

.activeTab {
  background:red;
  color:white;
}

<div id="component_projector_askforproduct">
6666666666666
</div>
<div id="opinions_58676">
qqqqqqqqqqqqqq
</div>

<div id="component_projector_opinions7">
11111111111111
</div>

Upvotes: 0

Mirko Vukušić

Reputation: 2121

What I would do is:

use object to store all your changes like:

var objChecks = {
  component_projector_askforproduct: "some_display_none_className",
  askforproduct_58676: "",
  longdescription_58676: ""
}

then create function to process it (pass objChecks to it):

function processChecks(checks) {
  Object.getOwnPropertyNames(objChecks).map(function(check){
    var el = document.getElementById(check);
    if (el) el.className=objChecks[check];
  })
}

change your HTML a bit. I noticed sometimes you change className and sometimes style.display. I'd make a new class that hides an element (same as display=none) which makes everything much neater.

Upvotes: 1