2
\$\begingroup\$

I have a form with two date (in format "YYYY-MM-DD") input fields 'Invoice sent'/'Payment received' and each of them has two checkboxes "pending"/"obsolete".

If one of the checkboxes is clicked, then the input field should be filled with '1111-11-11' or '0000-00-00' and the sibling checkboxes should be disabled.

This code works so far and does exactly that.

$(document).ready(function(){
  $("input.invoice-obsolete").click(toggle_checkbox);
  $("input.invoice-pending").click(toggle_checkbox);
  $("input.payment-obsolete").click(toggle_checkbox);
  $("input.payment-pending").click(toggle_checkbox);
  var invoice_sent = $(".invoice-sent").val();
  var payment_received = $(".payment-received").val();
  if (invoice_sent == '1111-11-11') {
  $("input.invoice-obsolete").attr("checked",true);
  }
  if (invoice_sent == '0000-00-00') {
    $("input.invoice-pending").attr("checked",true);
  }
  if (payment_received == '1111-11-11') {
    $("input.payment-obsolete").attr("checked",true);
  }
  if (payment_received == '0000-00-00') {
    $("input.payment-pending").attr("checked",true);
  }
});

function toggle_checkbox() {
  if($('input.invoice-obsolete').prop('checked')) {
    $("input.invoice-pending").attr("disabled", true);
    $('input.invoice-sent').val('1111-11-11');
    $("input.invoice-sent").attr("disabled", true);
  };

  if($('input.invoice-pending').prop('checked')) {
    $("input.invoice-obsolete").attr("disabled", true);
    $('input.invoice-sent').val('0000-00-00');
    $("input.invoice-sent").attr("disabled", true);
  };

  if(! $('input.invoice-obsolete').prop('checked')) {
    $("input.invoice-pending").removeAttr("disabled");
  }

  if(! $('input.invoice-pending').prop('checked')) {
    $("input.invoice-obsolete").removeAttr("disabled");
  }

  if ( (! $("input.invoice-pending").is(':checked')) && (! $("input.invoice-obsolete").is(':checked')) ) {
    $("input.invoice-sent").attr("disabled", false);
  }

  if($('input.payment-obsolete').prop('checked')) {
    $("input.payment-pending").attr("disabled", true);
    $('input.payment-received').val('1111-11-11');
    $("input.payment-received").attr("disabled", true);
  };

  if($('input.payment-pending').prop('checked')) {
    $("input.payment-obsolete").attr("disabled", true);
    $('input.payment-received').val('0000-00-00');
    $("input.payment-received").attr("disabled", true);
  };

  if(! $('input.payment-obsolete').prop('checked')) {
    $("input.payment-pending").removeAttr("disabled");
  }

  if(! $('input.payment-pending').prop('checked')) {
    $("input.payment-obsolete").removeAttr("disabled");
  }

  if ( (! $("input.payment-pending").is(':checked')) && (! $("input.payment-obsolete").is(':checked')) ) {
    $("input.payment-received").attr("disabled", false);
  }
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

<div>
  <label for="form-horizontal-text">Invoice sent</label>
  <input placeholder="YYYY-MM-DD" class="invoice-sent" type="text" name="job[invoice_sent]" id="job_invoice_sent" readonly="readonly">
  <div>
    <input name="job[invoice_sent_checkbox]" type="hidden" value="0"><input class="invoice-obsolete" type="checkbox" value="1" name="job[invoice_sent_checkbox]" id="job_invoice_sent_checkbox"> 
    <span class="invoice-sent">obsolete</span>
    <input name="job[invoice_sent_checkbox]" type="hidden" value="0"><input class="invoice-obsolete" type="checkbox" value="1" name="job[invoice_sent_checkbox]" id="job_invoice_sent_checkbox" disabled=""> 
    <span>pending</span>
  </div>
</div>

<div>
  <label for="form-horizontal-text">Payment received</label>
  <input placeholder="YYYY-MM-DD" class="payment-received" type="text" name="job[payment_received]" id="job_payment_received" readonly="readonly">
  <div>
    <input name="job[payment_received_checkbox]" type="hidden" value="0"><input class="payment-obsolete" type="checkbox" value="1" name="job[payment_received_checkbox]" id="job_payment_received_checkbox"> 
    <span class="invoice-sent">obsolete</span>
    <input name="job[payment_received_checkbox]" type="hidden" value="0"><input class="payment-pending" type="checkbox" value="1" name="job[payment_received_checkbox]" id="job_payment_received_checkbox"> 
    <span>pending</span>
  </div>
</div>

How can I refactor the code, so that I avoid unnecessary duplication?

\$\endgroup\$
2
\$\begingroup\$

There seem to be some bugs in the code:

  • JS code mentions input.invoice-pending, but there is no such element.
  • id attribute values should be unique on page.
  • name attribute values should also be unique within a form.

Suggestions for refactoring:

  • There are two parts with similar behavior: one for invoice, one for payment. Instead of prefixing each class name with invoice- or payment-, assign that class to the containing div and drop the prefix from elements inside:

    <div class="invoice">
        <input class="date" />
        <input class="obsolete" />
        <input class="pending" />
    </div>
    <div class="payment">
        <input class="date" />
        <input class="obsolete" />
        <input class="pending" />
    </div>
    

    Then you can create a generic function for handling both parts of the form:

    function initForm($container) {
        $container.find("input.obsolete").click(toggle_checkbox);
        ...
    }
    

    And then call it for both parts:

    $(document).ready(function(){
        initForm($("invoice"));
        initForm($("payment"));
    });
    
  • Instead of always performing a query like $('input.invoice-pending'), you can store the result to a variable and then use the variable:

    // Use plural form to show that we select multiple elements
    var $obsoleteInvoices = $('input.invoice-obsolete');
    
\$\endgroup\$

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.