Two duration widgets for a break timer, with ± buttons

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



Break length
-
5
+


Session length
-
25
+



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

StackExchange Code Review Q#112421, answer score: 2