0
\$\begingroup\$

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.

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;
        }
    }
}
<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>

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.

Thanks for any help!

\$\endgroup\$
1
  • \$\begingroup\$ I would also consider using radio buttons instead of a checkbox. \$\endgroup\$ Apr 7 at 14:22

2 Answers 2

3
\$\begingroup\$
  1. Your for loop in setPrice has a lot of code that does not vary between iterations of the loop. The loop only needs to add or remove the relevant class property, and the other code should be outside the loop. A couple of helper functions may be useful to take care of this.
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");
    }
}
  1. For the rest of the code, write a single function that updates values taking into account the status of all checkboxes. This will be triggered whenever anything changes in the form. This may mean some very minor inefficiencies (updating the innerHTML of bcValue when it hasn't actually changed) but will dramatically simplify the code. (If your page does eventually become much more complicated, you could write some extra code to check whether relevant values have changed before updating them on the page. It will still be simpler and easier to maintain than having every tick-box trigger a different function which independently updates the same set of values.)
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;
}
  1. It's usually better to set event listeners within javascript instead of in the HTML, making it easier to change, add more complicated behaviors, and having all of the logic in one place.
document.getElementById("toggle").addEventListener("click", update);
document.getElementById("package").addEventListener("click", update);
for (const id of ["toggle", "package"]) {
    document.getElementById(id).addEventListener("click", update);
}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks, this worked brilliantly! \$\endgroup\$ Apr 8 at 1:04
1
\$\begingroup\$

I put your code in a JSBin to test it out, but I'm not seeing anything happen in setPrice(), so I looked at setPackage().

The first thing you can do in setPackage is take out the four lines where you set the innerHTML and put one copy of that line after the if/then/else that sets the price.

I also think that you wrote **var** price = <some value> several times, but meant to write just price = <some value> as price is declared earlier.

Finally, do you know about the ternary operator (?:) in JavaScript? It's very useful for if/then/else constructions.

With these three suggestions, your function setPackage() could look like this:

function setPackage(){
  var cond1 = document.getElementById("package").checked === true
  var cond2 = document.getElementById("toggle").checked === true

  price = cond1 
      ? (cond2 ? both : both * 10) 
      : (cond2 ? single : single * 10)

  document.getElementById("price").innerHTML = price;
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ No need to nest the ternaries here - the OP's code actually does price = (cond1 ? both : single) * (cond2 ? 1 : 10); \$\endgroup\$
    – Bergi
    Apr 8 at 1:09
  • \$\begingroup\$ And drop the === true. \$\endgroup\$
    – Teepeemm
    Apr 8 at 2:19
  • \$\begingroup\$ Thanks for both your comments. Much cleaner. \$\endgroup\$ Apr 9 at 11:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.