Creating a function when user clicks on element to toggle another element

Question

I use a lot of "when user clicks on element then another element will be displayed as visible/hidden... aka toggle".

So I'm trying to make a general function to abide the DRY principal.

Before jumping into the code, I just wanted to state that there are still bugs. I just want to know before continuing if writing a function like the one below is necessary or if I should just ignore the DRY in this case and have lots of separate toggle functions. Or if it's necessary, would there be a more efficient method?

JSFiddle

*Brief Description of Code

-There are 4 parameters

• The first two are mandatory, the last two are optional
• The index parameter is a string of numbers ex. "1.0","1"
• The reason I need the last two parameters is because I have elements that can't be easily targeted. For example, they don't have an #id. So to grab those generic elements, I use getElementsByTagName

• Then I'll use the index parameter to get the element in the array.

• index parameter: The number before '.' is the index of the clickElement and the number after '.' is the index of the toggleElement. clickElement = 1. toggleElement = 0.

function toggleClick(clickElement, toggleElement, condition, index) { 
    var toggleIndex;
    var clickIndex;

    if(condition === false){
        if(index.indexOf('.') != -1){
            //indexOf returns -1 if '.' is not found. 
            // Checks to see if two index is entered
            indexArray = index.split('.');
            clickIndex = indexArray[0];
            toggleIndex = indexArray[1];
            clickElement = document.getElementsByTagName(clickElement)[clickIndex];
            toggleElement = document.getElementsByTagName(toggleElement)[toggleIndex];
        }else{
            //If there is no '.', that means only one index was entered.
            // By function requirement, it should be the index of clickElement

            clickElement = document.getElementsByTagName(clickElement)[index];
        }
    }
    $(clickElement).click(function(){
            $(toggleElement).toggle();
        });
    } 

This is one of my first functions, and so I need some guidance on whether I'm writing bad code. Yes, this might be broad because there may be many different ways to write this function. But I just need to know if there's a more efficient way or if my way is okay.

Answer 1

I know you're trying to be flexible here, but your approach comes at a cost: The API isn't understandable. I can see you're trying to toggle the second parameter element with the first parameter as the toggle, But I don't understand exactly how the third or fourth works, if it's even needed. It's also a weird convention.

Reflect on these three things here:

• The point of programming is having stuff done easier.
• The point of a programming language is to do programming in that's understandable to humans.
• The point of an API is to "lift" (abstract) the programming language.

Fail on one of those, and the rest before it breaks apart (and things like the great flood, alien invasions, world war 3 happen). So before you design an API, or even just a function, think about the users/developers using it. It might be you in the distant future.

---

Now off to your code. Seeing you're using jQuery, a similar feat could have been done through this piece of code. I do notice you said "elements that can't be easily targeted. For example, they don't have an #id." - jQuery still can target them using your standard CSS selectors (here's a few to remember), and some custom jQuery selectors (which I think is a superset of CSS selectors, if I'm not mistaken).

. The :eq() part is optional, but it does what you need, select specific indices.

// Assuming delegation for future elements
$(document).on('click','li:eq(0)',function(){
  $('div:eq(0)').toggle();
});

It's as simple as "Get the first of all <li> and toggle the first of all <div>". Simple enough. But what if you wanted your own simple function for it and not some jQuery mashup that is copy-paste bait? Something like this could be done too:

;(function ($) {
  $.fn.customToggle = function (trigger, target, event) {
    // Let's add a twist, custom functions. Assume click if not defined.
    event = event || 'click';
    this.on(event, trigger, function () {
      $(target).toggle();
    });
  }
}(jQuery));

// Still, future-proof, let's support future elements
$(document).customToggle('li:eq(0)', 'div:eq(0)');
$(document).customToggle('li:eq(1)', 'div:eq(1)','mouseover');

There you have it. Now you'd wonder "That's not any different from my code" - exactly. It's the same. However, there's one important bit I changed - it's the way I selected the elements and indices. I used CSS selectors - which every developer who came across CSS or jQuery should already know.

---

Here are other suggestions on how you could structure the API:

// Chained:
$(document)
  .customToggle('li:eq(0)', 'div:eq(0)')
  .customToggle('li:eq(2)', 'div:eq(4)')
  .customToggle('li:eq(3)', 'div:eq(9)')

// Map
$(document).customToggle({
  'li:eq(0)' : 'div:eq(0)',
  'li:eq(2)' : 'div:eq(4)',
  'li:eq(3)' : 'div:eq(9)'
});

Answer 2

I feel like this function will cause more problems than it's supposed to solve. Here is why:

A function should do a specific task, that is self descriptive by the function name. Having a function that does a lot of different things depending on it's parameters is not easy to use and read.

Unfortunately toggleClick does not do what it says, and it uses up brain matter every time you have to use it.

If I somehow ended up having to work with this code i'd have to run the if statements in my head before writing them as parameters to the function.

If the user clicks the first element, I should toggle the third child of that element,
but only if this variable is TRUE, otherwise the parent itself should toggle instead

If I had to use something like this, it is such a specific use case that I'd propably write the code I just thought about, instead of trying to figure out what this function does.

What I'd do:

If I can edit the markup, or the interacting elements are not decided dynamically, I'd set IDs to the elements I want to interact and do a simple

$('#myelement').on('click', function(){
      $('#target_element').toggle();
});

It might be 3 lines long, but you know what it does the moment you read it. If you want to mimic the use of your toggleClick you can try this instead:

// When I click the 2nd child of #my_element...
$('#my_element :eq(0)').on('click', function(){
    // Toggle the fifth child of the #target_element
    $('#target_element :eq(4)').toggle();
});