3
\$\begingroup\$

I am beginner on Javascript and i made a pomodoro clock for freecodecamp project with vanilla Javascript.

I was read few articles about modular and functional javascript and i want to use these techniques. But when i start writing code, I find it difficult to use these techniques. I will appreciated if anyone review my code and refactor it with modular patterns. Then I can compare the two codes, and that helps me a lot. And if there are bad implementations, please tell me my mistakes.

var addSession = document.querySelector('.add_session');
var decSession = document.querySelector('.dec_session');

var addBreak = document.querySelector('.add_break');
var decBreak = document.querySelector('.dec_break');

var sessionInput = parseInt(document.querySelector('.session_min').innerHTML);
document.querySelector('.session_min').innerHTML = sessionInput;
document.getElementById('counter').innerHTML = sessionInput;


var breakInput = document.querySelector('.break_min').innerHTML;
	breakInput = parseInt(breakInput);
document.querySelector('.break_min').innerHTML = breakInput;

var counterVal = document.getElementById('counter').innerHTML;
	counterVal = parseInt(counterVal);
	counterVal = sessionInput;
	counterVal = counterVal * 60;
document.getElementById('counter').innerHTML = counterVal;
document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;



var startBtn = document.querySelector('.start');
var resetBtn = document.querySelector('.reset');
var breakBtn = document.querySelector('.break');




addSession.onclick = function(e) {
	sessionInput += 5;
	document.querySelector('.session_min').innerHTML = sessionInput;
	counterVal = sessionInput * 60;
	document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
};

decSession.onclick = function(e) {
	if (sessionInput > 5) {
		sessionInput -= 5;
		document.querySelector('.session_min').innerHTML = sessionInput;
		counterVal = sessionInput * 60;
		document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
	}
};

addBreak.onclick = function(e) {
	breakInput += 1;
	document.querySelector('.break_min').innerHTML = breakInput;
	counterVal = breakInput * 60;
	document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
};

decBreak.onclick = function(e) {
	if (breakInput > 1) {
		breakInput -= 1;
		document.querySelector('.break_min').innerHTML = breakInput;
		counterVal = breakInput * 60;
		document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
	}
};



function timer() {

 	counterVal -= 1;
 	if (counterVal % 60 >= 10) {
		document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" +  counterVal % 60;
 	} else {
 		document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
 	}
 	if (counterVal === 0) {
 		clearInterval(counter);
 		startBtn.innerHTML = 'Start';
 	}
}

startBtn.addEventListener('click', function() {
	
	if (startBtn.innerHTML === 'Start' && counterVal !== 0) {
		breakBtn.disabled = true;
		startBtn.innerHTML = 'Pause';
		counter = setInterval(timer, 1000);

	} else if (startBtn.innerHTML === 'Pause' && counterVal > 0) {
		breakBtn.disabled = false;
		startBtn.innerHTML = 'Start';
		clearInterval(counter);
	}

}, false);

resetBtn.addEventListener('click', function() {
	if (startBtn.innerHTML !== 'Pause') {
		counterVal = sessionInput;
		counterVal = counterVal * 60;
		document.getElementById('counter').innerHTML = counterVal;
		document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
	}

	if (breakBtn.innerHTML === "Work") {
		breakBtn.innerHTML = "Break";
	}
}, false);


breakBtn.addEventListener('click', function(){

	if (breakBtn.innerHTML === "Break") {
		addBreak.disabled = false;
		decBreak.disabled = false;
		addSession.disabled = true;
		decSession.disabled = true;
		breakBtn.innerHTML = "Work";
		counterVal = breakInput;
		counterVal = counterVal * 60;
		document.getElementById('counter').innerHTML = counterVal;
		document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
	} else if (breakBtn.innerHTML === "Work") {
		addBreak.disabled = true;
		decBreak.disabled = true;
		addSession.disabled = false;
		decSession.disabled = false;
		breakBtn.innerHTML = "Break";
		counterVal = sessionInput * 60;
		document.getElementById('counter').innerHTML = counterVal;
		document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
	}
	
}, false);
<div class="clock_wrapper">
	<h1>Pomodoro Clock</h1>
	<div class="clock">
		<h2 id="counter"></h2>
		<div class="buttons_wrapper">
			<button class="start">Start</button>
			<button class="reset">Reset</button>
			<button class="break">Break</button>
		</div>
		<hr/>
	</div>
	<div class="session_wrap">
		<h3>Session Time</h3>
		<div class="session_time">
			<button class="dec_session bt">-</button>
			<div class="session_min">25</div>
			<button class="add_session bt">+</button>
		</div>
		
	</div>

	<div class="break_wrap">
		<h3>Break Time</h3>
		<div class="break_time">
			<button disabled class="dec_break bt">-</button>
			<div class="break_min">5</div>
			<button disabled class="add_break bt">+</button>
		</div>
	</div>
	
</div>

\$\endgroup\$
3
\$\begingroup\$

First of all delete marker from HTMl ;-).

First bad thing in your code is a global scope polution. Currently every var and functions 'lives' in global. Check this: https://stackoverflow.com/questions/10525582/why-are-global-variables-considered-bad-practice

var addSession = document.querySelector('.add_session');
var decSession = document.querySelector('.dec_session');

var addBreak = document.querySelector('.add_break');
var decBreak = document.querySelector('.dec_break');

var sessionInput = parseInt(document.querySelector('.session_min').innerHTML);

Bad idea to get state values from html. It should be oposite to this aproach. For example var sessionInput = 25; And then sett it in html.

document.querySelector('.session_min').innerHTML = sessionInput;
document.getElementById('counter').innerHTML = sessionInput;

Be more consistent. You can use document.querySelector('#counter').;

var breakInput = document.querySelector('.break_min').innerHTML;
    breakInput = parseInt(breakInput);
document.querySelector('.break_min').innerHTML = breakInput;

So you got some states in app to meanage. The good idea is to put them together into one object called state. It will make debuging a piece of cake. For example:

var clockState = {
  breakTime = 0,
  sessionTime = 0,
  counterVal = 0
}  

Next you can use them like this:

someFunction( clockState.breakTime );

This below is quite messy:

var counterVal = document.getElementById('counter').innerHTML;
    counterVal = parseInt(counterVal);
    counterVal = sessionInput;
    counterVal = counterVal * 60;
document.getElementById('counter').innerHTML = counterVal;
document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;

Below better way to solve task like this:

counter =  document.querySelector('#counter');
clockState.counterVal = 25 * 60;
counter.innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;

All variables should be declared at the begining. It makes easier to find them later ;-).

var startBtn = document.querySelector('.start');
var resetBtn = document.querySelector('.reset');
var breakBtn = document.querySelector('.break');

This is old way to meanage events in JS.

addSession.onclick = function(e) {
    sessionInput += 5;
    document.querySelector('.session_min').innerHTML = sessionInput;
    counterVal = sessionInput * 60;
    document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
};

Today people are using addEventListener:

function modifySession(){/* Doing the work*/}
addSession.addEventListener('click',modifySession)

//

decSession.onclick = function(e) {
    if (sessionInput > 5) {
        sessionInput -= 5;
        document.querySelector('.session_min').innerHTML = sessionInput;
        counterVal = sessionInput * 60;
        document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
    }
};

addBreak.onclick = function(e) {
    breakInput += 1;
    document.querySelector('.break_min').innerHTML = breakInput;
    counterVal = breakInput * 60;
    document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
};

decBreak.onclick = function(e) {
    if (breakInput > 1) {
        breakInput -= 1;
        document.querySelector('.break_min').innerHTML = breakInput;
        counterVal = breakInput * 60;
        document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
    }
};



function timer() {

    counterVal -= 1;
    if (counterVal % 60 >= 10) {
        document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" +  counterVal % 60;
    } else {
        document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
    }
    if (counterVal === 0) {
        clearInterval(counter);
        startBtn.innerHTML = 'Start';
    }
}

startBtn.addEventListener('click', function() {

    if (startBtn.innerHTML === 'Start' && counterVal !== 0) {
        breakBtn.disabled = true;
        startBtn.innerHTML = 'Pause';
        counter = setInterval(timer, 1000);

    } else if (startBtn.innerHTML === 'Pause' && counterVal > 0) {
        breakBtn.disabled = false;
        startBtn.innerHTML = 'Start';
        clearInterval(counter);
    }

}, false);

resetBtn.addEventListener('click', function() {
    if (startBtn.innerHTML !== 'Pause') {
        counterVal = sessionInput;
        counterVal = counterVal * 60;
        document.getElementById('counter').innerHTML = counterVal;
        document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
    }

    if (breakBtn.innerHTML === "Work") {
        breakBtn.innerHTML = "Break";
    }
}, false);


breakBtn.addEventListener('click', function(){

    if (breakBtn.innerHTML === "Break") {
        addBreak.disabled = false;
        decBreak.disabled = false;
        addSession.disabled = true;
        decSession.disabled = true;
        breakBtn.innerHTML = "Work";
        counterVal = breakInput;
        counterVal = counterVal * 60;
        document.getElementById('counter').innerHTML = counterVal;
        document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
    } else if (breakBtn.innerHTML === "Work") {
        addBreak.disabled = true;
        decBreak.disabled = true;
        addSession.disabled = false;
        decSession.disabled = false;
        breakBtn.innerHTML = "Break";
        counterVal = sessionInput * 60;
        document.getElementById('counter').innerHTML = counterVal;
        document.getElementById('counter').innerHTML = Math.floor(counterVal / 60) + ":" + "0" + counterVal % 60;
    }

}, false);

Currently your application is about 130 lines long. Try to make it 65. And learn the DRY principle: https://en.wikipedia.org/wiki/Don%27t_repeat_yourself.

If you are looking for challenges then try to write simple calculator.

\$\endgroup\$
  • \$\begingroup\$ thank you. I will try to apply what you say. By the way i made a simple calculator before pomodoro clock \$\endgroup\$ – erenesto May 26 '17 at 11:52

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.