4
\$\begingroup\$

I have been learning javascript for some time and have just finished my first piece of code that not only actually does something, but is also longer than a few lines of code. It most likely is pretty terrible though (but works!). I would appreciate any help, insight or/and pointers.

General idea was to create a module, that I can import, will generate HTML and do the timer things, and expose simple API while keeping most of the guts private (trying to wrap my head around general idea of modules and some popular patterns).

var timer_module = function (wrkTime, brkTime, parentEl) {

var locRef = {
        workTime: wrkTime ? wrkTime : 25,
        breakTime: brkTime ? brkTime : 10,
        seconds: 0,
        isPlaying: false,
        isBreak: false
    },
    seconds = locRef.seconds,
    minutes = locRef.workTime,
    pubAcc = {},
    secondsHandle, sdiv, mdiv, resetButton, startPauseButton;

(function render_DOM_elements(){
    var array_of_elements = [
        {tag: 'section', id: 'tm_container', parent: parentEl, initialContent: ''},
        {tag: 'div', id: 'tm_minutes', parent: 'tm_container', initialContent: ''},
        {tag: 'div', id: 'tm_seconds', parent: 'tm_container', initialContent: ''},
        {tag: 'div', id: 'tm_bcontainer', parent: 'tm_container', initialContent: ''},
        {tag: 'button', id: 'tm_playStopButton', parent: 'tm_bcontainer',
            initialContent: '<i class="fa fa-play-circle-o" aria-hidden="true"></i>'},
        {tag: 'button', id: 'tm_resetButton', parent: 'tm_bcontainer',
            initialContent: '<i class="fa fa-stop-circle-o" aria-hidden="true"></i>'}
    ];
    function create_element(elementObject){
        var el = document.createElement(elementObject.tag);
        var parent = document.getElementById(elementObject.parent);
        el.setAttribute('id', elementObject.id);
        el.innerHTML = elementObject.initialContent;
        if(!parent){
            document.body.appendChild(el)
        } else {
            parent.appendChild(el)
        }
    }
    array_of_elements.forEach(create_element);

    sdiv = document.getElementById('tm_seconds');
    mdiv = document.getElementById('tm_minutes');
    startPauseButton = document.getElementById('tm_playStopButton');
    resetButton = document.getElementById('tm_resetButton');
})();

//update the dom/display and format(zero-pad) the time
function updateTime() {
    sdiv.innerHTML = seconds < 10 ? '0' + seconds.toString() : seconds;
    mdiv.innerHTML = minutes < 10 ? '0' + minutes.toString() : minutes;
}

//change the guts of buttons, start/pause
function play_reset_change() {
    var play_icon = '<i class="fa fa-play-circle-o" aria-hidden="true"></i>',
        pause_icon = '<i class="fa fa-pause-circle-o" aria-hidden="true"></i>';
    startPauseButton.innerHTML = locRef.isPlaying ? pause_icon : play_icon
}

//steInterval guts
function minusSec() {
    if (minutes === 0 && seconds === 0) {
        if (!locRef.isBreak) {
            locRef.isBreak = true;
            minutes = locRef.breakTime;
            seconds = 0;
            updateTime();
        } else {
            locRef.isBreak = true;
            minutes = locRef.workTime;
            seconds = 0;
            updateTime();
        }
    } else if (minutes !== 0 && seconds === 0) {
        seconds = 59;
        minutes--;
        updateTime();
    } else {
        seconds--;
        updateTime();
    }
}
//initial time display
updateTime();

pubAcc.start = function () {
    locRef.isPlaying = true;
    //updateTime();
    play_reset_change();
    secondsHandle = setInterval(minusSec, 1000);
};
pubAcc.pause = function () {
    locRef.isPlaying = false;
    play_reset_change();
    clearInterval(secondsHandle);
};
pubAcc.reset = function () {
    locRef.isPlaying = false;
    locRef.isBreak = false;
    clearInterval(secondsHandle);
    minutes = locRef.workTime;

    seconds = 0;
    play_reset_change();
    updateTime();
};

startPauseButton.addEventListener('click', function () {
    if (!locRef.isPlaying) {
        pubAcc.start();
    } else {
        pubAcc.pause();
    }
});
resetButton.addEventListener('click', function () {
    pubAcc.reset();
});

return pubAcc;
};

var timer1 = timer_module(15,7,'i_want_pomodoro_here');
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! Consider making a live demo: press Ctrl-M in the question editor. \$\endgroup\$ Commented Apr 29, 2017 at 17:31

1 Answer 1

1
\$\begingroup\$

I've put the your code into a Code Snippet below, as well as a jsFiddle so anyone can easily see how it runs.

var timer_module = function(wrkTime, brkTime, parentEl) {

  var locRef = {
      workTime: wrkTime ? wrkTime : 25,
      breakTime: brkTime ? brkTime : 10,
      seconds: 0,
      isPlaying: false,
      isBreak: false
    },
    seconds = locRef.seconds,
    minutes = locRef.workTime,
    pubAcc = {},
    secondsHandle, sdiv, mdiv, resetButton, startPauseButton;

  (function render_DOM_elements() {
    var array_of_elements = [{
      tag: 'section',
      id: 'tm_container',
      parent: parentEl,
      initialContent: ''
    }, {
      tag: 'div',
      id: 'tm_minutes',
      parent: 'tm_container',
      initialContent: ''
    }, {
      tag: 'div',
      id: 'tm_seconds',
      parent: 'tm_container',
      initialContent: ''
    }, {
      tag: 'div',
      id: 'tm_bcontainer',
      parent: 'tm_container',
      initialContent: ''
    }, {
      tag: 'button',
      id: 'tm_playStopButton',
      parent: 'tm_bcontainer',
      initialContent: '<i class="fa fa-play-circle-o" aria-hidden="true"></i>'
    }, {
      tag: 'button',
      id: 'tm_resetButton',
      parent: 'tm_bcontainer',
      initialContent: '<i class="fa fa-stop-circle-o" aria-hidden="true"></i>'
    }];

    function create_element(elementObject) {
      var el = document.createElement(elementObject.tag);
      var parent = document.getElementById(elementObject.parent);
      el.setAttribute('id', elementObject.id);
      el.innerHTML = elementObject.initialContent;
      if (!parent) {
        document.body.appendChild(el)
      } else {
        parent.appendChild(el)
      }
    }
    array_of_elements.forEach(create_element);

    sdiv = document.getElementById('tm_seconds');
    mdiv = document.getElementById('tm_minutes');
    startPauseButton = document.getElementById('tm_playStopButton');
    resetButton = document.getElementById('tm_resetButton');
  })();

  //update the dom/display and format(zero-pad) the time
  function updateTime() {
    sdiv.innerHTML = seconds < 10 ? '0' + seconds.toString() : seconds;
    mdiv.innerHTML = minutes < 10 ? '0' + minutes.toString() : minutes;
  }

  //change the guts of buttons, start/pause
  function play_reset_change() {
    var play_icon = '<i class="fa fa-play-circle-o" aria-hidden="true"></i>',
      pause_icon = '<i class="fa fa-pause-circle-o" aria-hidden="true"></i>';
    startPauseButton.innerHTML = locRef.isPlaying ? pause_icon : play_icon
  }

  //steInterval guts
  function minusSec() {
    if (minutes === 0 && seconds === 0) {
      if (!locRef.isBreak) {
        locRef.isBreak = true;
        minutes = locRef.breakTime;
        seconds = 0;
        updateTime();
      } else {
        locRef.isBreak = true;
        minutes = locRef.workTime;
        seconds = 0;
        updateTime();
      }
    } else if (minutes !== 0 && seconds === 0) {
      seconds = 59;
      minutes--;
      updateTime();
    } else {
      seconds--;
      updateTime();
    }
  }
  //initial time display
  updateTime();

  pubAcc.start = function() {
    locRef.isPlaying = true;
    //updateTime();
    play_reset_change();
    secondsHandle = setInterval(minusSec, 1000);
  };
  pubAcc.pause = function() {
    locRef.isPlaying = false;
    play_reset_change();
    clearInterval(secondsHandle);
  };
  pubAcc.reset = function() {
    locRef.isPlaying = false;
    locRef.isBreak = false;
    clearInterval(secondsHandle);
    minutes = locRef.workTime;

    seconds = 0;
    play_reset_change();
    updateTime();
  };

  startPauseButton.addEventListener('click', function() {
    if (!locRef.isPlaying) {
      pubAcc.start();
    } else {
      pubAcc.pause();
    }
  });
  resetButton.addEventListener('click', function() {
    pubAcc.reset();
  });

  return pubAcc;
};

var timer1 = timer_module(15, 7, 'i_want_pomodoro_here');
<!DOCTYPE html>
<html>

<head>
  <meta name="description">
  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width">
  <title>JS Bin</title>
  <link href="https://maxcdn.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.min.css" rel="stylesheet" type="text/css" />
</head>

<body>
  <div id="i_want_pomodoro_here">
  </div>
</body>

</html>

My first piece of advice is to try to avoid overusing the comma notation for declaring variables:

var locRef = {
        workTime: wrkTime ? wrkTime : 25,
        breakTime: brkTime ? brkTime : 10,
        seconds: 0,
        isPlaying: false,
        isBreak: false
    },
    seconds = locRef.seconds,
    minutes = locRef.workTime,
    pubAcc = {},
    secondsHandle, sdiv, mdiv, resetButton, startPauseButton;

This is perfectly valid JavaScript. But it looks a bit confusing at first glance, since you go from one line per variable to multiple variables per line, all strung together with commas. There's no one correct way to do this, but in this case I would stick with one var per line.

var locRef = {
    workTime: wrkTime ? wrkTime : 25,
    breakTime: brkTime ? brkTime : 10,
    seconds: 0,
    isPlaying: false,
    isBreak: false
};
var seconds = locRef.seconds;
var minutes = locRef.workTime;
var pubAcc = {};
var secondsHandle, sdiv, mdiv, resetButton, startPauseButton;

It's a style issue, so feel free to ignore my advice if you feel that the above code doesn't look right to you.

\$\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.