how to not make my bootstrap 4 dropdown sort-by code an if hell-CodePudding

I'm a self-studying newbie to javascript/jquery, and i want to simplify/shorten(but readable) my working code but i can't think of any way. the problem is that i used too much if statements and i think the code could be more better but i don't know how.

it works as a sorting mechanic where you can only select 2 items (1 category and 1 order) and #sort_text will output it like "ID•ASC".

var for_button_text_1 = 'ID';
var for_button_text_2 = 'ASC';

$('#sort_text').html(for_button_text_1   '•'   for_button_text_2);

$('#ul_sort a').click(function(e) {
    e.stopPropagation(); //so it won't close after 1 item is selected

    let for_query = '';
    let element_id = $(this).attr('id');

    if (element_id == 1) {
        $('.sort_cat').removeClass('active');
        $('#'   element_id).addClass('active');
        for_query = 'id';
        for_button_text_1 = 'ID';
    } else if (element_id == 2) {
        $('.sort_cat').removeClass('active');
        $('#'   element_id).addClass('active');
        for_query = 'title';
        for_button_text_1 = 'Title';
    } else if (element_id == 3) {
        $('.sort_cat').removeClass('active');
        $('#'   element_id).addClass('active');
        for_query = 'created_at';
        for_button_text_1 = 'Date added';
    } else if (element_id == 4) {
        $('.sort_cat').removeClass('active');
        $('#'   element_id).addClass('active');
        for_query = 'updated_at';
        for_button_text_1 = 'Date edited';
    } else {
        if (element_id == 5) {
            $('.sort_order').removeClass('active');
            $('#'   element_id).addClass('active');
            for_query = 'asc';
            for_button_text_2 = 'ASC';
        } else if (element_id == 6) {
            $('.sort_order').removeClass('active');
            $('#'   element_id).addClass('active');
            for_query = 'desc';
            for_button_text_2 = 'DESC';
        }
    }

    $('#sort_text').html(for_button_text_1   '•'   for_button_text_2);
});

<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" integrity="sha384-zCbKRCUGaJDkqS1kPbPd7TveP5iyJE0EjAuZQTgFLD2ylzuqKfdKlfG/eSrtxUkn" crossorigin="anonymous">
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/font/bootstrap-icons.css">
<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/jquery.slim.min.js" integrity="sha384-DfXdz2htPH0lsSSs5nCTpuj/zy4C OGpamoFVy38MVBnE IbbVYUew OrCXaRkfj" crossorigin="anonymous"></script>
<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/js/bootstrap.bundle.min.js" integrity="sha384-fQybjgWLrvvRgtW6bFlB7jaZrFsaBXjsOMm/tB9LTS58ONXgqbR9W8oWht/amnpF" crossorigin="anonymous"></script>
<div >
    <a  role="button" data-toggle="dropdown"
    aria-expanded="false">
        <span id="icon_sort_change" ></span>
        Sort by
    </a>
    <ul id="ul_sort" >

        <a id="1" >ID</a>
        <a id="2" >Title</a>
        <a id="3" >Date added</a>
        <a id="4" >Date edited</a>

        <li ></li> <!-- .dropdown-divider -->

        <a id="5" >
            Ascending
            <span ></span>
        </a>
        <a id="6" >
            Descending
            <span ></span>
        </a>
    </ul>
</div>
<span id="sort_text"></span>

note: var for_button_text1&2 values are supposed to be displayed inside the a.dropdown-toggle's innerHTML but i changed my mind but not the variable names lol

CodePudding user response：

This part of code is repeated:

    $('.sort_cat').removeClass('active');
    $('#'   element_id).addClass('active');
    for_query = 'created_at';
    for_button_text_1 = 'Date added';

So you can simplify it by creating a functin, like:

const repeatedCode = function(forQuery, forButton) {
    $('.sort_cat').removeClass('active');
    $('#'   element_id).addClass('active');
    for_query = forQuery;
    for_button_text_1 = forButton;
}

and use it across your code. like:

repeatedCode('created_at', 'Date added')

I think you can simplify it more, but its a good start in my opinion.

Improvements you can do:

Use === instead of ==
you can use template literal instead of concatenating with plus sign. like

${for_button_text_1} • ${for_button_text_2}

CodePudding user response：

You can add both "for_query" and the text to be displayed as data attributes.
You can then use jquery's .data() method to get those values.

You can then write a dynamic javascript to take care of any future changes to the categories. Try something like this

$(document).ready(function(){
    $('#sort_text').html('ID•ASC');

    $('#ul_sort a').click(function(e) {
        e.preventDefault(); // prevent a tags default action
        e.stopPropagation(); // so it won't close after 1 item is selected

        let for_query = $(this).data('query'); // get the value for `for_query`

        // check if the clicked element is a category. if it is, make it active
        // else the clicked element is for ordering. perform same action as one above
        if( $(this).hasClass('sort_cat') ){
            $('.sort_cat').removeClass('active');
            $(this).addClass('active');
        }else{
            $('.sort_order').removeClass('active');
            $(this).addClass('active');
        }

        // get the texts of both selected category and order and display them
        let category = $('.sort_cat.active').data('text');
        let order = $('.sort_order.active').data('text');
        $('#sort_text').html(category   '•'   order);
    });
})

<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" integrity="sha384-zCbKRCUGaJDkqS1kPbPd7TveP5iyJE0EjAuZQTgFLD2ylzuqKfdKlfG/eSrtxUkn" crossorigin="anonymous">
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/font/bootstrap-icons.css">
<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/jquery.slim.min.js" integrity="sha384-DfXdz2htPH0lsSSs5nCTpuj/zy4C OGpamoFVy38MVBnE IbbVYUew OrCXaRkfj" crossorigin="anonymous"></script>
<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/js/bootstrap.bundle.min.js" integrity="sha384-fQybjgWLrvvRgtW6bFlB7jaZrFsaBXjsOMm/tB9LTS58ONXgqbR9W8oWht/amnpF" crossorigin="anonymous"></script>
<div >
    <a  role="button" data-toggle="dropdown"
    aria-expanded="false">
        <i id="icon_sort_change" ></i>
        Sort by
    </a>
    <div id="ul_sort" >
        <a href="#"  data-text="ID" data-query="id">ID</a>
        <a href="#"  data-text="Title" data-query="title">Title</a>
        <a href="#"  data-text="Date added" data-query="created_at">Date added</a>
        <a href="#"  data-text="Date edited" data-query="updated_at">Date edited</a>

        <div ></div>

        <a href="#"  data-text="ASC" data-query="asc">
            Ascending
            <i ></i>
        </a>
        <a href="#"  data-text="DESC" data-query="desc">
            Descending
            <i ></i>
        </a>
    </div>
</div>
<span id="sort_text"></span>

CodePudding user response：

Here is another way of condensing the logic into a few functions. Similar to OP's post I am also using global variables. I would strongly recommend to find other ways of communicating the information. It also struck me that there is only one query-related variable for_query holding sort category and sort order in turn. This might also need some revision.

const opts=["","id,ID,","title,Title,","created_at,Date added,","updated_at,Date edited,","asc,,ASC","desc,,DESC"].map(s=>s.split(","));
function setVars(id){
  let o=opts[id];
  ["for_query","for_button_text_1","for_button_text_2"].forEach((b,i)=>o[i]&&(window[b]=o[i]));
}
function showSort(){
  $('#sort_text').html(for_button_text_1   '•'   for_button_text_2);
}
$('#ul_sort').on("click","a",function(e) {
  e.stopPropagation();
  setVars(this.id);            
  showSort();
  console.log(for_query);
});
setVars(1);setVars(5);showSort();

<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" integrity="sha384-zCbKRCUGaJDkqS1kPbPd7TveP5iyJE0EjAuZQTgFLD2ylzuqKfdKlfG/eSrtxUkn" crossorigin="anonymous">
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/font/bootstrap-icons.css">
<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/jquery.slim.min.js" integrity="sha384-DfXdz2htPH0lsSSs5nCTpuj/zy4C OGpamoFVy38MVBnE IbbVYUew OrCXaRkfj" crossorigin="anonymous"></script>
<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/js/bootstrap.bundle.min.js" integrity="sha384-fQybjgWLrvvRgtW6bFlB7jaZrFsaBXjsOMm/tB9LTS58ONXgqbR9W8oWht/amnpF" crossorigin="anonymous"></script>
<div >
    <a  role="button" data-toggle="dropdown"
    aria-expanded="false">
        <span id="icon_sort_change" ></span>
        Sort by
    </a>
    <ul id="ul_sort" >

        <a id="1" >ID</a>
        <a id="2" >Title</a>
        <a id="3" >Date added</a>
        <a id="4" >Date edited</a>

        <li ></li> <!-- .dropdown-divider -->

        <a id="5" >
            Ascending
            <span ></span>
        </a>
        <a id="6" >
            Descending
            <span ></span>
        </a>
    </ul>
</div>
<span id="sort_text"></span>