4
\$\begingroup\$

I'm hoping for some input on if my code is DRY or how I could have made it more efficient. I am pretty new in terms of jQ/JavaScript and this took me far longer than I care to admit. I will post the CodePen and also the entire JavaScript (Unsure which is better in terms of formatting.) Forked CodePen so new changes won't effect it

var timer;

$(document).ready(function(){
var working = false;
var active = '';
var workTime = 0;
var breakTime = 0;
checkStatus();

  //Test Vars

/*  Alternate Method of 'checkStatus Function'

working ? $('#pause').addClass('disabled')&& $('#start').removeClass('disabled'): $('#start').addClass('disabled')&& $('#pause').removeClass('disabled');
 */

  //Controls if a button is disabled based on status of timer
function checkStatus() {
  if (!working) {
    $('#start').removeClass('disabled');
    $('#pause').addClass('disabled');
    $('#reset').addClass('disabled');
    } else {
    $('#pause').removeClass('disabled');
    $('#reset').removeClass('disabled');
    $('#start').addClass('disabled');
    }
}
//End Check Status
    //Function to Show Time  !!Finished!!
    function showTime(time) {
   var min = Math.floor(time/60);
   var sec = Math.round(time%60);
      if (sec < 10) {
        sec = '0' + sec
      }
      var timeString = min+':'+sec
      $('#msg').text("Hey! You've only got    "+timeString+" left to go!")
     }
  //End showTime
  //Enables the timer  !!Mostly Finished!!
function startTimer() {
  $('.jumbotron').css('visibility', 'visible');
  return setInterval(function() {
    console.log("Work Timer...")
    workTime--;
    if (workTime < 0) {
      clearInterval(timer);
      timer = breakTimer();
    } else {
      showTime(workTime);
    }
  }, 1000);
}
  //End Timer
  //What Happens when #start is pressed
 function start() {     
   if (working == true){ //This keeps it from being spammable
      return
   } //Else
  workTime = $('#work').val()*60;
    breakTime = $('#break').val()*60;
   working = true;
   checkStatus();    
    timer = startTimer();
 } 
  
  
  //What Happens when #pause/resume is pressed
  function pause() {
    clearInterval(timer);
    $('.resume').unbind().click(resume);
    $('#pause').html('Resume');
    $('#pause').addClass('resume');
    $('#pause').removeClass('pause');
    $('.resume').click(resume);
   }
  
  
  function resume(){  
    $('#pause').unbind().click(pause);
    $('#pause').html('Pause');
    $('#pause').addClass('pause');
    $('#pause').removeClass('resume');
    timer = startTimer();
    }
  //What happens when #reset is pressed
  function reset() {
   clearInterval(timer);
    working = false;
    workTime = 0;
    breakTime = 0;
    checkStatus();
    $('.jumbotron').css('visibility', 'hidden');
    $('#msg').html("");
  }
  //Break Timer
  function breakTimer() {
    $('.jumbotron').css('visibility', 'visible');
    return setInterval(function() {
      console.log("Break Timer...");
    breakTime--;
    if (breakTime < 0) {
      clearInterval(timer);
      working = false;
      start();
    } else {
      showTime(breakTime);
    }
  }, 1000);
}
  //Button Association
  $('#start').click(start);
    $('#work').keypress(function(e) {
    if(e.which == 13) {
       start();
    }
});
  //This Makes Enter Work as well to Start
  $('.pause').click(pause);
  $('#reset').click(reset); 
  
}); //End of DocReady
.jumbotron{
  visibility: hidden;
}
<script src="//cdnjs.cloudflare.com/ajax/libs/jquery/2.1.3/jquery.min.js"></script>
<script src="//maxcdn.bootstrapcdn.com/bootstrap/3.3.5/js/bootstrap.min.js"></script>

<div class='container'>
  <h1 class='text-center text-primary'>Pomodoro Clock(?)!</h1>
  <div class='row'>
    <div class='col-xs-6'>
      <label for='work'>Work time: </label>
          <input autofocus id='work' type='text' min=0 max=200 value='25' > mins.
    </div>
    <div class='col-xs-6'>
          <label for='break'>Break time: </label>
          <input id='break' type='text' min=0 max=100 value=5> mins.
    </div>
  </div>
  <div class='row'>
    <div class="col-sm-4">
      <button id='start' class='btn btn-primary center-block'>Start Work</button>
    </div>
    <div class="col-sm-4">
      <button id='pause' class='btn btn-primary center-block pause'>Pause</button>
    </div>
    <div class="col-sm-4">
      <button id='reset' class='btn btn-primary center-block'>Reset</button>
    </div>
  </div><!--Row Closer-->
  

<!--Active Timer-->
  <div class='jumbotron'>
    <h1 id='msg' class='text-center'></h1>
    <h1 id='time' class='text-center'></h1>
  </div>
<!--Active Timer Close-->
  <h1 id='alert' class='text-center'></h1>

</div><!--Container Closer-->

\$\endgroup\$
1
\$\begingroup\$

There's so much i would change. You should use prototype it would be better programming, because the way you coded for example "pause" is a function in the global scope. So if you later would program something else with the same name the "pause" function would be overriden.

Another way would be to create a jquery plugin but i guess it's a lot harder since you're just a beginner.

But for a first change you could for example put pause and resume a same function.

function pause() {
  if($('#pause').hasClass('pause')){
    $('#pause').html('Pause');
    $('#pause').removeClass('pause');
  }else{
    clearInterval(timer);
    $('#pause').html('Resume');
    $('#pause').addClass('pause');
  }
} 

StartTimer and BreakTimer could be put together also. Since both only do a decrement of time.(I'll let you find out how :P )
Use timeout instead of interval. The reason behind it is that you can call it whenever you want and don't need to clear it when you want to pause.

Try to breakdown your code into smaller chunks. It makes everything easier to read and you get reusable code.

example i would break down showTime in 2 functions
One function returning the time "min:sec" and the other returning text with the time.

\$\endgroup\$
  • \$\begingroup\$ Really appreciate the input - Yea, a beginner for sure. I'll try to figure out the merging of start/breakTime (I assume doing the same method would suffice. An if/else built in is how I was originally going to do it.) I originally had used timeout instead of interval but I heard a lot of people saying it's 'wrong' because it apparently has issues if it isn't in your primary focus panel - The timer mis-aligns. But I can understand why setInterval wouldn't be a good idea in something more massive. \$\endgroup\$ – Donavin Sep 1 '15 at 22:43
  • \$\begingroup\$ Your main problem here was that you created global functions. Since it's somehow a plugin you're creating. The reason to create plugins is that you can than reuse the code everywhere by using the plugin file (you would have to generate the html over the plugin also). The other thing that beginners often do is create code that works but that's not reusable. If you check big libraries code you will see that often there are really small functions that have almost no code. A function should do one thing and not many. \$\endgroup\$ – Marcio Sep 2 '15 at 3:17
  • \$\begingroup\$ Don't try to generate text and create a timer inside one function. Break it in 2 functions (talking about showtime as an example) If you do so than you will have moments where you ask yourself."Ohh i need a function that returns time in "min:seconds" and i wrote it in that js file. Let me just copy paste the function and reuse it." While if you would write it mixed with the text etc... you would ask yourself the same question but than you have to read the code and find out which parts to copy paste and probably also modify the code if things where mixed with it. \$\endgroup\$ – Marcio Sep 2 '15 at 3:18

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.