Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

It looks really good! Here are a few points;


I don't like the arguments in your setLimit and init functions, especially because some of them are optional.

I would prefer an Object argument in this case.

function setLimit(limits) {
 finalLimit = limits.final;
 warnLimit = limits.warn;
 errorLimit = limits.error;
}
this.setLimit({
 final: 10,
 warn: 5,
 error: 8
});

This makes it obvious at call time which argument is which.


Your var declarations are inconsistent here:

var finalLimit = null,
 warnLimit = null,
 errorLimit = null;
var max = 59;
var interval = null;

Pick either to comma separate them or not. Not both.


In a couple of places you use == instead of ===. Generally we stick to === for reasons reasons.


You CSS selectors are very generic. This is generally a bad thing as they will easily collide with other styles the moment you integrate it into a real page.

I favour BEM syntax, which would make it look something like this;

<div class="timer">
 <div class="timer__labels"> 
 <span id="timer__hours-label">00</span>
 : <span id="timer__minutes-label">00</span>
 : <span id="timer__seconds-label">00</span>
 </div>
 <button class="timer__btn js-start-timer">Start</button>
 <button class="timer__btn js-end-timer">Stop</button>
 <button class="timer__btn js-reset-timer">Reset</button>
</div>

Note I've also removed your onclick handlers in favour of interaction specific classes. I prefix my classnames with js- if they are used solely for JavaScript selectors. This helps decouple the CSS and JavaScript.

In your code you then attach the handlers;

document.querySelectorAll('.js-start-timer')[0]
 .addEventListener('click', startTimer);

It looks really good! Here are a few points;


I don't like the arguments in your setLimit and init functions, especially because some of them are optional.

I would prefer an Object argument in this case.

function setLimit(limits) {
 finalLimit = limits.final;
 warnLimit = limits.warn;
 errorLimit = limits.error;
}
this.setLimit({
 final: 10,
 warn: 5,
 error: 8
});

This makes it obvious at call time which argument is which.


Your var declarations are inconsistent here:

var finalLimit = null,
 warnLimit = null,
 errorLimit = null;
var max = 59;
var interval = null;

Pick either to comma separate them or not. Not both.


In a couple of places you use == instead of ===. Generally we stick to === for reasons.


You CSS selectors are very generic. This is generally a bad thing as they will easily collide with other styles the moment you integrate it into a real page.

I favour BEM syntax, which would make it look something like this;

<div class="timer">
 <div class="timer__labels"> 
 <span id="timer__hours-label">00</span>
 : <span id="timer__minutes-label">00</span>
 : <span id="timer__seconds-label">00</span>
 </div>
 <button class="timer__btn js-start-timer">Start</button>
 <button class="timer__btn js-end-timer">Stop</button>
 <button class="timer__btn js-reset-timer">Reset</button>
</div>

Note I've also removed your onclick handlers in favour of interaction specific classes. I prefix my classnames with js- if they are used solely for JavaScript selectors. This helps decouple the CSS and JavaScript.

In your code you then attach the handlers;

document.querySelectorAll('.js-start-timer')[0]
 .addEventListener('click', startTimer);

It looks really good! Here are a few points;


I don't like the arguments in your setLimit and init functions, especially because some of them are optional.

I would prefer an Object argument in this case.

function setLimit(limits) {
 finalLimit = limits.final;
 warnLimit = limits.warn;
 errorLimit = limits.error;
}
this.setLimit({
 final: 10,
 warn: 5,
 error: 8
});

This makes it obvious at call time which argument is which.


Your var declarations are inconsistent here:

var finalLimit = null,
 warnLimit = null,
 errorLimit = null;
var max = 59;
var interval = null;

Pick either to comma separate them or not. Not both.


In a couple of places you use == instead of ===. Generally we stick to === for reasons.


You CSS selectors are very generic. This is generally a bad thing as they will easily collide with other styles the moment you integrate it into a real page.

I favour BEM syntax, which would make it look something like this;

<div class="timer">
 <div class="timer__labels"> 
 <span id="timer__hours-label">00</span>
 : <span id="timer__minutes-label">00</span>
 : <span id="timer__seconds-label">00</span>
 </div>
 <button class="timer__btn js-start-timer">Start</button>
 <button class="timer__btn js-end-timer">Stop</button>
 <button class="timer__btn js-reset-timer">Reset</button>
</div>

Note I've also removed your onclick handlers in favour of interaction specific classes. I prefix my classnames with js- if they are used solely for JavaScript selectors. This helps decouple the CSS and JavaScript.

In your code you then attach the handlers;

document.querySelectorAll('.js-start-timer')[0]
 .addEventListener('click', startTimer);
added 1144 characters in body
Source Link
Jivings
  • 2.3k
  • 14
  • 17

It looks really good! Here are a few points;


I don't like the arguments in your setLimit and init functions, especially because some of them are optional.

I would prefer an Object argument in this case.

function setLimit(limits) {
 finalLimit = limits.final;
 warnLimit = limits.warn;
 errorLimit = limits.error;
}
this.setLimit({
 final: 10,
 warn: 5,
 error: 8
});

This makes it obvious at call time which argument is which.


Your var declarations are inconsistent here:

var finalLimit = null,
 warnLimit = null,
 errorLimit = null;
var max = 59;
var interval = null;

Pick either to comma separate them or not. Not both.


In a couple of places you use == instead of ===. Generally we stick to === for reasons.


You CSS selectors are very generic. This is generally a bad thing as they will easily collide with other styles the moment you integrate it into a real page.

I favour BEM syntax , which would make it look something like this;

<div class="timer">
 <div class="timer__labels"> 
 <span id="timer__hours-label">00</span>
 : <span id="timer__minutes-label">00</span>
 : <span id="timer__seconds-label">00</span>
 </div>
 <button class="timer__btn js-start-timer">Start</button>
 <button class="timer__btn js-end-timer">Stop</button>
 <button class="timer__btn js-reset-timer">Reset</button>
</div>

Note I've also removed your onclick handlers in favour of interaction specific classes. I prefix my classnames with js- if they are used solely for JavaScript selectors. This helps decouple the CSS and JavaScript.

In your code you then attach the handlers;

document.querySelectorAll('.js-start-timer')[0]
 .addEventListener('click', startTimer);

It looks really good! Here are a few points;


I don't like the arguments in your setLimit and init functions, especially because some of them are optional.

I would prefer an Object argument in this case.

function setLimit(limits) {
 finalLimit = limits.final;
 warnLimit = limits.warn;
 errorLimit = limits.error;
}
this.setLimit({
 final: 10,
 warn: 5,
 error: 8
});

This makes it obvious at call time which argument is which.


Your var declarations are inconsistent here:

var finalLimit = null,
 warnLimit = null,
 errorLimit = null;
var max = 59;
var interval = null;

Pick either to comma separate them or not. Not both.


In a couple of places you use == instead of ===. Generally we stick to === for reasons.

It looks really good! Here are a few points;


I don't like the arguments in your setLimit and init functions, especially because some of them are optional.

I would prefer an Object argument in this case.

function setLimit(limits) {
 finalLimit = limits.final;
 warnLimit = limits.warn;
 errorLimit = limits.error;
}
this.setLimit({
 final: 10,
 warn: 5,
 error: 8
});

This makes it obvious at call time which argument is which.


Your var declarations are inconsistent here:

var finalLimit = null,
 warnLimit = null,
 errorLimit = null;
var max = 59;
var interval = null;

Pick either to comma separate them or not. Not both.


In a couple of places you use == instead of ===. Generally we stick to === for reasons.


You CSS selectors are very generic. This is generally a bad thing as they will easily collide with other styles the moment you integrate it into a real page.

I favour BEM syntax , which would make it look something like this;

<div class="timer">
 <div class="timer__labels"> 
 <span id="timer__hours-label">00</span>
 : <span id="timer__minutes-label">00</span>
 : <span id="timer__seconds-label">00</span>
 </div>
 <button class="timer__btn js-start-timer">Start</button>
 <button class="timer__btn js-end-timer">Stop</button>
 <button class="timer__btn js-reset-timer">Reset</button>
</div>

Note I've also removed your onclick handlers in favour of interaction specific classes. I prefix my classnames with js- if they are used solely for JavaScript selectors. This helps decouple the CSS and JavaScript.

In your code you then attach the handlers;

document.querySelectorAll('.js-start-timer')[0]
 .addEventListener('click', startTimer);
Source Link
Jivings
  • 2.3k
  • 14
  • 17

It looks really good! Here are a few points;


I don't like the arguments in your setLimit and init functions, especially because some of them are optional.

I would prefer an Object argument in this case.

function setLimit(limits) {
 finalLimit = limits.final;
 warnLimit = limits.warn;
 errorLimit = limits.error;
}
this.setLimit({
 final: 10,
 warn: 5,
 error: 8
});

This makes it obvious at call time which argument is which.


Your var declarations are inconsistent here:

var finalLimit = null,
 warnLimit = null,
 errorLimit = null;
var max = 59;
var interval = null;

Pick either to comma separate them or not. Not both.


In a couple of places you use == instead of ===. Generally we stick to === for reasons.

default

AltStyle によって変換されたページ (->オリジナル) /