1
\$\begingroup\$

I have written the following Javascript code which increments and decrements 3 separate inputs by 1:

//INCREMENTS/DECREMENTS BEDROOMS BY 1

function plusOneBedrooms() {
    var addRoom = document.getElementById('rooms_amount_bedroom');
    value = parseInt(addRoom.getAttribute('value'), 10) + 1;
    addRoom.setAttribute('value', value);
    addRoom.innerHTML = value;

}

function minusOneBedrooms() {
    var subtractRoom = document.getElementById('rooms_amount_bedroom');
    value = parseInt(subtractRoom.getAttribute('value'), 10) - 1;
    subtractRoom.setAttribute('value', value);
    subtractRoom.innerHTML = value;

}

//INCREMENTS/DECREMENTS BATHROOMS BY 1
function plusOneBathrooms() {
    var addRoom = document.getElementById('rooms_amount_bathrooms');
    value = parseInt(addRoom.getAttribute('value'), 10) + 1;
    addRoom.setAttribute('value', value);
    addRoom.innerHTML = value;

}

function minusOneBathrooms() {
    var subtractRoom = document.getElementById('rooms_amount_bathrooms');
    value = parseInt(subtractRoom.getAttribute('value'), 10) - 1;
    subtractRoom.setAttribute('value', value);
    subtractRoom.innerHTML = value;

}
//INCREMENTS/DECREMENTS KITCHENS BY 1
function plusOneKitchens() {
    var addRoom = document.getElementById('rooms_amount_kitchens');
    value = parseInt(addRoom.getAttribute('value'), 10) + 1;
    addRoom.setAttribute('value', value);
    addRoom.innerHTML = value;

}

function minusOneKitchens() {
    var subtractRoom = document.getElementById('rooms_amount_kitchens');
    value = parseInt(subtractRoom.getAttribute('value'), 10) - 1;
    subtractRoom.setAttribute('value', value);
    subtractRoom.innerHTML = value;

}
    <div class="room_type_wrap">

        <small class="counter_title counter_opacity1">Bedrooms</small>
        <hr class="counter_title_underline">
        <div class="counter_wrap">
            <div class="subtractRoom" onclick="minusOneBedrooms()">
                <img src="./assets/images/arrow_down.png" alt="" class="counter_down_arrow">
            </div>
            <input type="number" class="room_count" id="rooms_amount_bedroom" autocomplete="off" value="0">
            <div class="addRoom" onclick="plusOneBedrooms()">
                <img src="./assets/images/arrow_up_white.png" alt="" class="counter_up_arrow">
            </div>
        </div>

        <small class="counter_title counter_opacity2">Bathrooms</small>
        <hr class="counter_title_underline">
        <div class="counter_wrap">
            <div class="subtractRoom" onclick="minusOneBathrooms()">
                <img src="./assets/images/arrow_down.png" alt="" class="counter_down_arrow">
            </div>
            <input type="number" class="room_count" id="rooms_amount_bathrooms" autocomplete="off" value="0">
            <div class="addRoom" onclick="plusOneBathrooms()">
                <img src="./assets/images/arrow_up_white.png" alt="" class="counter_up_arrow">
            </div>
        </div>

        <small class="counter_title counter_opacity3">Kitchens</small>
        <hr class="counter_title_underline">
        <div class="counter_wrap">
            <div class="subtractRoom" onclick="minusOneKitchens()">
                <img src="./assets/images/arrow_down.png" alt="" class="counter_down_arrow">
            </div>
            <input type="number" class="room_count" id="rooms_amount_kitchens" autocomplete="off" value="0">
            <div class="addRoom" onclick="plusOneKitchens()">
                <img src="./assets/images/arrow_up_white.png" alt="" class="counter_up_arrow">
            </div>
        </div>

    </div>

I would like to be able to have the same functionality but feel I could have probably done this in a simpler way. I'm new to Javascript/JQuery so any suggestions will be appreciated. Thanks!

\$\endgroup\$
5
  • \$\begingroup\$ Thanks, i've edited my question accordingly. \$\endgroup\$ May 26 '20 at 13:47
  • \$\begingroup\$ @DelroyBrown your code does not appear to function as intended: addRoom.innerHTML = value looks like a mistake. \$\endgroup\$
    – user120242
    May 26 '20 at 18:52
  • \$\begingroup\$ Also, I think you might just be looking for the HTML input type number? No javascript/jQuery needed. \$\endgroup\$
    – Graipher
    May 26 '20 at 18:56
  • 1
    \$\begingroup\$ @Graipher He's already using type=number. I think his code is just broken, and it isn't clear what his code was meant to achieve. \$\endgroup\$
    – user120242
    May 26 '20 at 19:05
  • \$\begingroup\$ @Graipher Hi, the intent of the code is to increment/decrement an input value by 1 when the addRoom and subtractRoom buttons are clicked. The code works I'm just having trouble trying to simplify the method i've used. \$\endgroup\$ May 26 '20 at 19:28
1
\$\begingroup\$

Some things could be said about the HTML (no need for hr, border will do; those imgs should be background images on buttons; this is obviously a list; those smalls should be labels; way to many divs; ...) but let me focus on te JS as you are asking.

First thing that jumps out is the repetition. Each function basically does the same thing, but for a different room and with a different amount. Not very DRY at all.

The easy solution would be to pass the room name and the change amount in the function as parameters, and now you only need one function. Something like this:

function changeValue(roomName, amount) { ...

You can them call your function from the onclick attribute like so:

<div ... onclick="changeValue('bedroom', -1)">

You can now add more rooms without having to change the javascript, and if you decide to change something in your logic you only have to change it once.

Second thing that was already noticed in the comments, you are changing the innerHtml of your inputs, but there is no such thing on inputs (there is no opening and closing tag with 'innerhtml' between them). Changing the value will suffice.

Also, you mention and tag jQuery, but this is vanilla JS, not a line off jQuery in there (which is fine btw!)

Let me close by showing you how my code would look if I had to write it:

HTML:

<ul id="rooms">
  <li>
    <label for="bathroom_count">bathrooms</label>
    <button data-amount="-1">-</button>
    <input type="number" id="bathroom_count" value="0" autocomplete="off"/>
    <button data-amount="1">+</button>
  </li>
  <li>
    <label for="bedroom_count">bedrooms</label>
    <button data-amount="-1">-</button>
    <input type="number" id="bedroom_count" value="0" autocomplete="off"/>
    <button data-amount="1">+</button>
  </li>
  <li>
    <label for="kitchen_count">kitchens</label>
    <button data-amount="-1">-</button>
    <input type="number" id="kitchen_count" value="0" autocomplete="off"/>
    <button data-amount="1">+</button>
  </li>
</ul>

jQuery:

  $('#rooms li').on('click', 'button', function(e) {
    e.preventDefault();
    var $button = $(e.currentTarget);
    var $input = $button.siblings('input');

    $input.val(parseInt($input.val(), 10) + parseInt($button.data('amount'), 10));
  });

I think the code should speak for itself, but feel free to ask if you want me to elaborate.

I hope you learned something from it and happy coding!

\$\endgroup\$
1
  • \$\begingroup\$ This answer is excellent! Thanks alot @Pevara, this has helped me a lot. \$\endgroup\$ May 27 '20 at 10:25

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.