Kelsey Brookes
Kelsey Brookes

Reputation: 123

How can I improve this javascript to simplify it and avoid repeating myself?

I've created a working price toggler, that also has an upsell remover/adder. While it works well, I'd like to understand how I could do it better, because I'm having to essentially run two compound if/else statements to account for the duration toggle and the upsell toggle.

HTML:

<div>
    <p>Annually</p>

    <!-- Toggle Button -->
    <label for="toggle">
        <!-- toggle -->
        <div class="relative">
            <!-- hidden input -->
            <input
                id="toggle"
                type="checkbox"
                class="hidden"
                onclick="setPrice()"
                data-sc-monthly="59"
                data-both-monthly="79"
                data-bc-monthly="20"
             />
            <!-- line -->
            <div></div>
            <!-- dot -->
            <div class="toggle_dot"></div>
        </div>
    </label>

    <p>Monthly</p>
</div>
<div>
    <div>
        <h3>Main Report</h3>
        <h4>$<span id="price">790</span></h4>
        <p><span class="annual">ANNUALLY</span><span class="month hidden">MONTHLY</span></p>
        <div>
            <input
            id="package"
            type="checkbox"
            onclick="setPackage()"
            checked
            />
            <h5>Include our special secondary coverage for just $<span id="bcValue">200</span></h5>
        </div>
        <a class="btn btn-success">
            JOIN NOW
        </a>
    </div>
</div>

JAVASCRIPT:

<script>
const pricing = document.querySelector('#toggle');

var single = pricing.dataset.scMonthly;
var both = pricing.dataset.bothMonthly;
var bc = pricing.dataset.bcMonthly;
var price = both;

function setPackage(){
    if (document.getElementById("package").checked == true) {
        if (document.getElementById("toggle").checked == true) {
            var price = both;
            document.getElementById("price").innerHTML = price;
        } else {
            var price = both * 10;
            document.getElementById("price").innerHTML = price;
        }
    } else {
        if (document.getElementById("toggle").checked == true) {
            var price = single;
            document.getElementById("price").innerHTML = price;
        } else {
            var price = single * 10;
            document.getElementById("price").innerHTML = price;
        }
    }
}

function setPrice() {
    var x = document.querySelectorAll('.annual');
    var y = document.querySelectorAll('.month');
    for (var i = 0; i < x.length; i++) {
        if (document.getElementById("toggle").checked == true) {
            x[i].classList.add('hidden');
            y[i].classList.remove('hidden');
            if (document.getElementById("package").checked == true) {
                document.getElementById("price").innerHTML = both;
            } else {
                document.getElementById("price").innerHTML = single;
            }
            document.getElementById("bcValue").innerHTML = bc;
        } else {
            x[i].classList.remove('hidden');
            y[i].classList.add('hidden');

            if (document.getElementById("package").checked == true) {
                document.getElementById("price").innerHTML = both * 10;
            } else {
                document.getElementById("price").innerHTML = single * 10;
            }
            document.getElementById("bcValue").innerHTML = bc * 10;
        }
    }
}
</script>

As you can see, each function has to check if the parameters controlled by the other function are set or not, and make adjustments accordingly.

My JS-fu is not strong enough yet to figure out how to improve this code overall.

Upvotes: 1

Views: 70

Answers (2)

Kelsey Brookes
Kelsey Brookes

Reputation: 123

Here's what I wound up using based on an answer from CodeReview:

const pricing = document.querySelector('#toggle');
var single = pricing.dataset.scMonthly;
var both = pricing.dataset.bothMonthly;
var bc = pricing.dataset.bcMonthly;
var price = both;

for (const id of ["toggle", "package"]) {
    document.getElementById(id).addEventListener("click", update);
}

function hideAll(query) {
    for (const element of document.querySelectorAll(query)) {
        element.classList.add("hidden");
    }
}
function showAll(query) {
    for (const element of document.querySelectorAll(query)) {
        element.classList.remove("hidden");
    }
}

function update() {
    const package = document.getElementById("package").checked ? both : single;
    let factor;
    if (document.getElementById("toggle").checked) {
        factor = 1;
        hideAll(".annual");
        showAll(".month");
    } else {
        factor = 10;
        showAll(".annual");
        hideAll(".month");
    }
    document.getElementById("price").innerHTML = package * factor;
    document.getElementById("bcValue").innerHTML = bc * factor;
}

Upvotes: 1

P&#225;draig Galvin
P&#225;draig Galvin

Reputation: 1155

Since you are always updating the same elements, you can work out the price before assigning the result:

function setPackage(){
    var price = document.getElementById("package").checked ? both : single;
    if (document.getElementById("toggle").checked) {
        price *= 10;
    }
    document.getElementById("price").innerHTML = price;
}

For the second function, you should move the logic that updates the price out of the loop to avoid repeating the same action unnecessarily.

function setPrice() {
    var showMonthly = document.getElementById("toggle").checked;
    var hideClass = showMonthly ? '.annual' : '.monthly'; // choose which info to hide

    // display the currently hidden info
    document.querySelectorAll('.hidden').forEach(function (el) {
        el.classList.remove('hidden');
    });
    document.querySelectorAll(hideClass).forEach(function (el) {
        el.classList.add('hidden');
    });

    var price = document.getElementById("package").checked ? both : single;
    var multiplier = showMonthly ? 1 : 10;
    document.getElementById("price").innerHTML = price * multiplier;
    document.getElementById("bcValue").innerHTML = bc * multiplier;
}

Upvotes: 2

Related Questions