JQuery Clyp Auto-embed

Question

This function finds links from Clyp.it and offers users a link to embed the content. Please let me know what you think of it. It works, but probably has some redundancies and inefficiencies.

$(document).ready(function() {
    var clyplinks = $('a[href^="https://clyp.it/"]').filter(function() {
        return this.innerHTML.match(/(https:\/\/clyp.it\/)([\w]{8})/);
    });
    $(clyplinks).each(function(index) {

        var x = null;
        var id = $(this).attr("href").split("it/")[1];
        var e = $('<a class="embed" id="' + id + index +'" href="#"> [embed]</a>').insertAfter($(this));
        e.click(function(evt) {
            evt.preventDefault();
            if (x != 1) {
                $(this).after('<div id="clyp' + id + index + '"><iframe width="259" height="160" src="https://clyp.it/' + id + '/widget" frameborder="0" ></iframe></div>')
                $(e).html('<a class="embed" id="' + id + index +'" href="#"> [remove]</a>')
                x = 1;
                console.log('embed' + x + ' ' + index);
            } else {
                $('#clyp' + id + index).detach();
                $(e).html('<a class="embed" id="' + id + index +'" href="#"> [embed]</a>')
                x = null;
                console.log('yay' + x + ' ' + index);

            }

        });

    });
});

<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.1/jquery.min.js"></script>
<a href="https://clyp.it/5e0dmibw" rel="nofollow" target="_blank">https://clyp.it/5e0dmibw</a>

JSFiddle:

https://jsfiddle.net/1svgq6g4/

Answer 1

Regex

The regex used is /(https:\/\/clyp.it\/)([\w]{8})/. Since, here you're checking if the string is URL and not using to extract data from string, capturing groups can be removed.

[\w]: Match word character(alphanumeric character and underscore). This can be written as \w by removing character class.

I'll also suggest to use word boundary and use of i-case-insensitive flag to match both uppercase and/or lowercase alphabets.

/\bhttps:\/\/clyp.it\/\w{8}\b/

match vs test

String#match is used to extract data from string. To check if string follows particular pattern, use RegExp#test. test is faster than match.

See regex.test V.S. string.match to know if a string matches a regular expression

$('a[href^="https://clyp.it/"]').filter(function() {
    return /https:\/\/clyp.it\/\w{8}/i.test(this.innerHTML);
});

Variable Naming

Variable names should be self-descriptive. I'll recommend to read https://github.com/ryanmcdermott/clean-code-javascript#variables.

1. clyplinks can be just links or clypLinks(camelCase)
2. x can be renamed to flag and used as boolean(true/false)
3. e should be el if you like short names or element. As it is jQuery wrapped element, it is common to use $ as prefix in variable name- $element.

Event binding

For single link, this will work. If there are multiple links on the page to bind events on dynamic elements event delegation can be used. When using event delegation, the code should be moved outside of each so that event is not bound multiple times.

The syntax is

$(parentElementSelector).on(event, childElements, callbackFunction)

In this case, as there is no common parent element which contain all the links, we can use document

$(document).on('click', '.embed', function(event) {

Using data-*

Instead of using a flag to store the status of link, HTML5 custom data-* can be used. So, on click of link, first check if variable value is set and depending on the value perform action. To read the value of data-* attribute use data().

// Inside click handler
if ($(this).data('state') === 'embedded') {
    // Already embedded
    // Remove
} else {
    // Not embedded
    // Add
}

// Change the value
$(this).data('state', function(i, value) {
    // switch the value
    return value === 'embedded' ? 'not embedded' : 'embedded';
});

After doing these changes, previous code will not work. Inside the handler $(e).html( is used to change the link text. Note that html() will set the innerHTML of the element and not replace it. If you inspect the Embed link by clicking on it, you'll notice that link is nested. To solve this, just use $(this).html('[remove]'). Also, note that $(this) inside event handler refer to the element on which the event has occurred.

jQuery Wrapping & Method Chaining

clyplinks is already jQuery collection of element. There is no need of wrapping it in jQuery again.

jQuery allows to chain most of the methods.

var clyplinks = $('a[href^="https://clyp.it/"]').filter(function() {
    return this.innerHTML.match(/(https:\/\/clyp.it\/)([\w]{8})/);
});
$(clyplinks).each(function(index) {

can be written as

$(someSelector).filter(function() {
    return ...;
}).each(function(index) {

This also eliminates clyplinks variable.

---

Below things are also important

1. Code Formatting: You may install plugin/package in editor/IDE
2. Missing Semi-colons: Use jslint/jshint