4
\$\begingroup\$

I'm sure I can optimise this code, but I can't find how in order to be generic. I'm not sure that .parent and .child combination is good in the long term here.

$(document).ready(function() {
  var lengthInit = $("#lengthSession").text();
  $("#timer").text(lengthInit);
  $("#dimBreak").click(function() {
    dimLength();
  });
  $("#augBreak").click(function() {
    augLength();
  });
  $("#dimSession").click(function() {
    dimLengthSession();
  });
  $("#augSession").click(function() {
    augLengthSession();
  });
});

function dimLength() {
  var length = parseInt($("#lengthBreak").text());
  length = length - 1;
  $("#lengthBreak").text(length);
}

function augLength() {
  var length = parseInt($("#lengthBreak").text());
  length += 1;
  $("#lengthBreak").text(length);
}

function dimLengthSession() {
  var length = parseInt($("#lengthSession").text());
  length = length - 1;
  $("#lengthSession").text(length);
}

function augLengthSession() {
  var length = parseInt($("#lengthSession").text());
  length += 1;
  $("#lengthSession").text(length);
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
  <div class="rowTime">
    <div class="inputTime">
      <h2 class="titre">Break length</h2>
      <button class="boutons" id="dimBreak">-</button>
      <span class="length" id="lengthBreak">5</span>
      <button class="boutons" id="augBreak">+</button>
    </div>
    <div class="inputTime">
      <h2 class="titre">Session length</h2>
      <button class="boutons" id="dimSession">-</button>
      <span class="length" id="lengthSession">25</span>
      <button class="boutons" id="augSession">+</button>
    </div>
  </div>

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for translating (some of) the symbols to English. Unfortunately, my fellow moderator pointed out that it was a mistake on my part to suggest to translate, and I agree (see our discussion in the chatroom). No need to touch this anymore, this is just for your information, and for the record. \$\endgroup\$
    – janos
    Dec 1, 2015 at 10:12

2 Answers 2

2
\$\begingroup\$

Extract common logic to helper method

These functions have duplicate logic, but different data:

function augLength() {
  var length = parseInt($("#lengthBreak").text());
  length += 1;
  $("#lengthBreak").text(length);
}

function augLengthSession() {
  var length = parseInt($("#lengthSession").text());
  length += 1;
  $("#lengthSession").text(length);
}

A simple improvement is to extract the common logic to a helper method:

function inc(selector) {
  var length = parseInt($(selector).text());
  length += 1;
  $(selector).text(length);
}

function augLength() {
  inc("#lengthBreak");
}

function augLengthSession() {
  inc("#lengthSession");
}

Parsing an int

Instead of:

  var length = parseInt($(selector).text());

Always when using parseInt, you should specify a radix parameter to indicate the base, for example for base-10:

  var length = parseInt($(selector).text(), 10);

And, what if the text cannot be parsed? It's good to make it default to something, say, 0, which you can achieve simply with:

  var length = parseInt($(selector).text(), 10) || 0;

DOM lookups are expensive

Instead of:

function inc(selector) {
  var length = parseInt($(selector).text(), 10) || 0;
  length += 1;
  $(selector).text(length);
}

It's better to do the DOM lookup once and reuse:

function inc(selector) {
  var dom = $(selector);
  var length = parseInt(dom.text(), 10) || 0;
  length += 1;
  dom.text(length);
}

Extracting more common logic

The inc helper can be extended to take care of the decrements too, by adding a second parameter:

function inc(selector, increment) {
  var dom = $(selector);
  var length = parseInt(dom.text(), 10) || 0;
  length += increment;
  dom.text(length);
}

function augLength() {
  inc("#lengthBreak", 1);
}

function augLengthSession() {
  inc("#lengthSession", 1);
}

function dimLength() {
  inc("#lengthBreak", -1);
}

function dimLengthSession() {
  inc("#lengthSession", -1);
}

Functions as parameters

You can do without the augLength, dimLength, augLengthSession, dimLengthSession functions, if you like, by using another helper that returns a function:

function makeIncrementer(selector, increment) {
  return function () { inc(selector, increment); };
}

Which then you can call directly from the ready block:

$(document).ready(function() {
  var lengthInit = $("#lengthSession").text();
  $("#timer").text(lengthInit);
  $("#dimBreak").click(makeIncrementer('#lengthBreak', -1));
  $("#augBreak").click(makeIncrementer('#lengthBreak', 1));      
  $("#dimSession").click(makeIncrementer('#lengthSession', -1));
  $("#augSession").click(makeIncrementer('#lengthSession', 1));
});
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Impressive answer, by its quickness and its details! Thanks very much \$\endgroup\$
    – jayjaypg22
    Dec 1, 2015 at 9:38
2
\$\begingroup\$

Since you tagged this question as , I think that it deserves a review taking HTML5 into account. HTML5 has an <input type="number"> input type that is designed just for this purpose. You should use it because:

  • It's semantically relevant. The whole widget is represented in HTML as just one element.
  • It provides a standard DOM interface through which your code can interact with the field.
  • You get all of the functionality that you implemented for free!
  • It results in a better user experience. For example, if you click and hold one of the arrows, it would continue to increment or decrement.
  • Out of the box, it would blend in with the look-and-feel of the user's desktop.
  • The spec reminds you that you can set options like a min="0" attribute that would be applicable to a timer.
  • It would be trivial to change it to, say, an <input type="range">, if you wanted to.

It's just this simple:

fieldset {
    border: none;
}
label {
    display: block;
    font-size: larger;
    font-weight: bold;
}
<form>
  <fieldset>
    <label for="lengthBreak">Break length</label>
    <input type="number" min="0" step="1" value="5" id="lengthBreak">
  </fieldset>
  <fieldset>
    <label for="lengthSession">Session length</label>
    <input type="number" min="0" step="1" value="25" id="lengthSession">
  </fieldset>
</form>

Note that I have replaced your <div>s and <h2>s with more semantically relevant <fieldset>s and <label>s.


What if you need it to work on a browser that doesn't support HTML5? What if you don't like the way the widget looks? Then you can use a shim to make it work the way you want. For example, you can use jQuery UI to customize the look.

I'd much rather maintain this little bit of non-critical JavaScript code than implement the widget from scratch. An additional benefit is that it integrates well with the jQuery philosophy — just one call to $('input[type=number]').mySpinner() instantiates both widgets.

$.widget('custom.mySpinner', $.ui.spinner, {
    _create: function() {
        this._super('_create');
        $(this.element).attr('type', 'text');
    },
});

$(function() {
    $('input[type=number]').mySpinner();
});
fieldset {
    border: none;
}
label {
    display: block;
    font-size: larger;
    font-weight: bold;
}

/* jQuery UI spinner visual customization */
.ui-spinner-button {
    height: 100% !important;
}
.ui-spinner-input {
    position: relative;
    left: 50%;
}
.ui-spinner-down {
    position: absolute;
    left: 0;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<link rel="stylesheet" href="https://ajax.googleapis.com/ajax/libs/jqueryui/1.11.4/themes/smoothness/jquery-ui.css">
<script src="https://ajax.googleapis.com/ajax/libs/jqueryui/1.11.4/jquery-ui.min.js"></script>

<form>
  <fieldset>
    <label for="lengthBreak">Break length</label>
    <input type="number" min="0" step="1" value="5" id="lengthBreak">
  </fieldset>
  <fieldset>
    <label for="lengthSession">Session length</label>
    <input type="number" min="0" step="1" value="25" id="lengthSession">
  </fieldset>
</form>

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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