Is there any more generic way for below code?


#1

Hello All,

I recently started coding. Please help me to write more generic way of writing below code.

function set_selection(elem)
	{
	switch(elem.id){
		case "div_SortCost" :
		if(elem.id == "div_SortCost")
		{
			if(btnSortNumber.elem !=null)
			{
		btnSortNumber.disactivateSelection();
			}
			if(btnSortCost.elem!=null)
			{
		btnSortCost.activateSelection();
			}
			if(btnSortHour.elem!=null)
			{
		btnSortHour.disactivateSelection();
			}
		}
		break;
		
		case "div_SortHour":
		if(btnSortNumber.elem!=null)
			{
		btnSortNumber.disactivateSelection();
			}
			if(btnSortCost.elem!=null)
			{
		btnSortCost.disactivateSelection();
			}
			if(btnSortHour.elem!=null)
			{
		btnSortHour.activateSelection();
			}
			break;
			
			case "div_SortNumber":
			if(btnSortNumber.elem!=null)
			{
		btnSortNumber.activateSelection();
			}
			if(btnSortHour.elem!=null)
			{
		btnSortHour.disactivateSelection();
			}
			if(btnSortCost.elem!=null)
			{
		btnSortCost.disactivateSelection();
			}
			break;
	}
	}

Thanks
Chandu


#2

I'll take a look and see what I can do.


#3

First of all the if comparison must go, as that's already taken care of in the switch statement.

    case "div_SortCost":
      // if (elem.id == "div_SortCost") {

Next up, we have several sections being deactivated and one being activated, so a good policy here is to do the deactivations first before activating other things.

      if (btnSortNumber.elem != null) {
        btnSortNumber.disactivateSelection();
      }
      if (btnSortHour.elem != null) {
        btnSortHour.disactivateSelection();
      }
      if (btnSortCost.elem != null) {
        btnSortCost.activateSelection();
      }

We can use a separate function to activate or deactivate an element.

function disactivate(btn) {
  if (btn.elem != null) {
    btn.disactivateSelection();
  }
}
function activate(btn) {
  if (btn.elem != null) {
    btn.activateSelection();
  }
}
...
    case "div_SortCost":
      disactivate(btnSortNumber);
      disactivate(btnSortHour);
      activate(btnSortCost);
      break;

I want to use only the activate in the case statement. To achieve that, the activate function must deactivate the other ones. I'll introduce some duplication to clean up the case statements, which will help us to find duplication elsewhere that can be cleaned up too.

function activateSortCost(btn) {
  disactivate(btnSortNumber);
  disactivate(btnSortHour);
  if (btn.elem != null) {
    btn.activateSelection();
  }
}
function activateSortHour(btn) {
  disactivate(btnSortNumber);
  disactivate(btnSortCost);
  if (btn.elem != null) {
    btn.activateSelection();
  }
}
function activateSortNumber(btn) {
  disactivate(btnSortHour);
  disactivate(btnSortCost);
  if (btn.elem != null) {
    btn.activateSelection();
  }
}

That lets us dramatically simplify the case statements:

  switch (elem.id) {
    case "div_SortCost":
      activateSortCost(btnSortCost);
      break;
    case "div_SortHour":
      activateSortHour(btnSortHour);
      break;
    case "div_SortNumber":
      activateSortNumber(btnSortNumber);
      break;
  }

With the activate functions, we need it to recognise that the function that we pass to it, isn't to be deactivated. That's something that can be done by placing all of the function names into an array, and filtering it.

var buttons = [btnSortNumber, btnSortHour, btnSortCost];
...
function activateSortCost(btn) {
  buttons
    .filter(button => button !== btn)
    .forEach(disactivate);
  if (btn.elem != null) {
    btn.activateSelection();
  }
}

That will work regardless of which button is passed to it, so we can remove the custom activate names and use only the one activate function instead:

function activate(btn) {
  buttons
    .filter(button => button !== btn)
    .forEach(disactivate);
  if (btn.elem != null) {
    btn.activateSelection();
  }
}
// function activateSortCost(btn) {
  // ...
// }
// function activateSortHour(btn) {
 //  ...
// }
// function activateSortNumber(btn) {
  // ...
}
...
    case "div_SortCost":
      // activateSortCost(btnSortCost);
      activate(btnSortCost);
      break;
    case "div_SortHour":
      // activateSortHour(btnSortHour);
      activate(btnSortHour);
      break;
    case "div_SortNumber":
      // activateSortNumber(btnSortNumber);
      activate(btnSortNumber);
      break;

If we name the buttons array contain objects with the div and the function, we can make this even more generic than it currently is.

var buttons = [
  {id: "btnSortCost", fn: btnSortCost},
  {id: "div_SortHour", fn: btnSortHour},
  {id: "div_SortNumber", fn: btnSortNumber}
];

The activate function needs to check the button.fn property

  buttons
    .filter(button => button.fn !== btn)

And the switch is not even needed now.

  activate(
    buttons.find(button => button.id === elem.id).fn
  );
  // switch (elem.id) {
  //   case "div_SortCost":
  //     activate(btnSortCost);
  //     break;
  //   case "div_SortHour":
  //     activate(btnSortHour);
  //     break;
  //   case "div_SortNumber":
  //     activate(btnSortNumber);
  //     break;
  // }

That gives us the following more generic code:

var buttons = [
  {id: "div_SortCost", fn: btnSortCost},
  {id: "div_SortHour", fn: btnSortHour},
  {id: "div_SortNumber", fn: btnSortNumber}
];

function disactivate(btn) {
  if (btn.elem != null) {
    btn.disactivateSelection();
  }
}
function activate(btn) {
  buttons
    .filter(button => button.fn !== btn)
    .forEach(disactivate);
  if (btn.elem != null) {
    btn.activateSelection();
  }
}
function set_selection(elem) {
  activate(
    buttons.find(button => button.id === elem.id).fn
  );
}

To make things even easier though (and they can get amazingly simple), knowing the HTML code can help to make things as simple as:

function deactivateButtons() {
    document.querySelectorAll("button").forEach(function (button) {
      button.classList.remove("activate");
    });
}
document.querySelectorAll("button").forEach(function (button) {
  button.addEventListener("click", function (evt) {
    deactivateButtons();
    evt.target.classList.add("activate");
  });
});

https://jsfiddle.net/pmw57/fu35wurv/1/


#4

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.