JavaScript flipping clock

Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
3
down vote
favorite
Here are two simple classes - Digit class maintains digit behaviour - primarily flipping animations, and Clock class handles everything that is related to time data.
I'm going to use this code for job application, please tell me if there are any improvements that could be done and what do you think of it.
Here is the link for working demo: https://abm0.github.io/flipping-clock.
class Digit
constructor( selector, value = 0 )
const digitEl = document.querySelector(selector);
this.flipperEls = digitEl.querySelectorAll('.flipper');
this.prevDigitEls = digitEl.querySelectorAll('.prev .digit');
this.nextDigitEls = digitEl.querySelectorAll('.next .digit');
this.value = value;
this.prevValue = null;
this.renderInitialValue();
setValue(nextValue)
this.prevValue = this.value;
this.value = nextValue;
if (this.value === this.prevValue) return;
this.flip();
renderInitialValue()
const
prevDigitEls,
nextDigitEls,
= this;
[...prevDigitEls, ...nextDigitEls].forEach(el => (el.innerHTML = this.value));
flip()
this.nextDigitEls.forEach(el => el.innerHTML = this.value);
this.flipperEls.forEach(el => el.classList.add('turned'));
setTimeout(() =>
this.prevDigitEls.forEach(el => (el.innerHTML = this.value));
this.flipperEls.forEach((el) =>
el.classList.remove('turned');
);
, 500);
class Clock
constructor(props)
const baseEl = document.querySelector("#clock");
const currentTime = this.getCurrentTime();
this.digits = [
'hours-tens',
'hours-ones',
'minutes-tens',
'minutes-ones',
'seconds-tens',
'seconds-ones'
];
this.buildDigits(currentTime);
getCurrentTime()
const date = new Date();
const time =
hours: date.getHours(),
minutes: date.getMinutes(),
seconds: date.getSeconds(),
;
this.formatValues(time);
return time;
formatValues(time)
Object.keys(time).forEach(key =>
if (key === "ampm") return;
let value = time[key];
if (parseInt(value) < 10)
time[key] = "0" + value;
time[key] = time[key].toString();
);
getDigitProps(digitName)
const type = digitName.split('-')[0];
const position = digitName.split('-')[1];
let positionIndex;
switch (position)
case 'tens':
positionIndex = 0;
break;
case 'ones':
positionIndex = 1;
break;
return type, position, positionIndex ;
buildDigits(time)
this.digits.forEach((digitName) =>
const type, position, positionIndex = this.getDigitProps(digitName);
const selector = `#$type .$position-digit`;
this[digitName] = new Digit(
selector,
value: time[type][positionIndex]
);
);
tick()
const time = this.getCurrentTime();
this.digits.forEach((digitName) =>
const type, positionIndex = this.getDigitProps(digitName);
this[digitName].setValue(time[type][positionIndex]);
);
const clock = new Clock();
setInterval(() =>
clock.tick();
, 1000);
javascript datetime animation dom timer
add a comment |Â
up vote
3
down vote
favorite
Here are two simple classes - Digit class maintains digit behaviour - primarily flipping animations, and Clock class handles everything that is related to time data.
I'm going to use this code for job application, please tell me if there are any improvements that could be done and what do you think of it.
Here is the link for working demo: https://abm0.github.io/flipping-clock.
class Digit
constructor( selector, value = 0 )
const digitEl = document.querySelector(selector);
this.flipperEls = digitEl.querySelectorAll('.flipper');
this.prevDigitEls = digitEl.querySelectorAll('.prev .digit');
this.nextDigitEls = digitEl.querySelectorAll('.next .digit');
this.value = value;
this.prevValue = null;
this.renderInitialValue();
setValue(nextValue)
this.prevValue = this.value;
this.value = nextValue;
if (this.value === this.prevValue) return;
this.flip();
renderInitialValue()
const
prevDigitEls,
nextDigitEls,
= this;
[...prevDigitEls, ...nextDigitEls].forEach(el => (el.innerHTML = this.value));
flip()
this.nextDigitEls.forEach(el => el.innerHTML = this.value);
this.flipperEls.forEach(el => el.classList.add('turned'));
setTimeout(() =>
this.prevDigitEls.forEach(el => (el.innerHTML = this.value));
this.flipperEls.forEach((el) =>
el.classList.remove('turned');
);
, 500);
class Clock
constructor(props)
const baseEl = document.querySelector("#clock");
const currentTime = this.getCurrentTime();
this.digits = [
'hours-tens',
'hours-ones',
'minutes-tens',
'minutes-ones',
'seconds-tens',
'seconds-ones'
];
this.buildDigits(currentTime);
getCurrentTime()
const date = new Date();
const time =
hours: date.getHours(),
minutes: date.getMinutes(),
seconds: date.getSeconds(),
;
this.formatValues(time);
return time;
formatValues(time)
Object.keys(time).forEach(key =>
if (key === "ampm") return;
let value = time[key];
if (parseInt(value) < 10)
time[key] = "0" + value;
time[key] = time[key].toString();
);
getDigitProps(digitName)
const type = digitName.split('-')[0];
const position = digitName.split('-')[1];
let positionIndex;
switch (position)
case 'tens':
positionIndex = 0;
break;
case 'ones':
positionIndex = 1;
break;
return type, position, positionIndex ;
buildDigits(time)
this.digits.forEach((digitName) =>
const type, position, positionIndex = this.getDigitProps(digitName);
const selector = `#$type .$position-digit`;
this[digitName] = new Digit(
selector,
value: time[type][positionIndex]
);
);
tick()
const time = this.getCurrentTime();
this.digits.forEach((digitName) =>
const type, positionIndex = this.getDigitProps(digitName);
this[digitName].setValue(time[type][positionIndex]);
);
const clock = new Clock();
setInterval(() =>
clock.tick();
, 1000);
javascript datetime animation dom timer
WithsetInterval( ...., 1000)you'll get your clock to tick once every second. Be aware, however, it's not said it will tick at the beginning of a second. It may tick at the end of each second as well, hence it can be effectively one second late. You may want to set the interval to100to reduce the possible delay in refresh to 1/10 of second.
â CiaPan
Jul 18 at 13:29
add a comment |Â
up vote
3
down vote
favorite
up vote
3
down vote
favorite
Here are two simple classes - Digit class maintains digit behaviour - primarily flipping animations, and Clock class handles everything that is related to time data.
I'm going to use this code for job application, please tell me if there are any improvements that could be done and what do you think of it.
Here is the link for working demo: https://abm0.github.io/flipping-clock.
class Digit
constructor( selector, value = 0 )
const digitEl = document.querySelector(selector);
this.flipperEls = digitEl.querySelectorAll('.flipper');
this.prevDigitEls = digitEl.querySelectorAll('.prev .digit');
this.nextDigitEls = digitEl.querySelectorAll('.next .digit');
this.value = value;
this.prevValue = null;
this.renderInitialValue();
setValue(nextValue)
this.prevValue = this.value;
this.value = nextValue;
if (this.value === this.prevValue) return;
this.flip();
renderInitialValue()
const
prevDigitEls,
nextDigitEls,
= this;
[...prevDigitEls, ...nextDigitEls].forEach(el => (el.innerHTML = this.value));
flip()
this.nextDigitEls.forEach(el => el.innerHTML = this.value);
this.flipperEls.forEach(el => el.classList.add('turned'));
setTimeout(() =>
this.prevDigitEls.forEach(el => (el.innerHTML = this.value));
this.flipperEls.forEach((el) =>
el.classList.remove('turned');
);
, 500);
class Clock
constructor(props)
const baseEl = document.querySelector("#clock");
const currentTime = this.getCurrentTime();
this.digits = [
'hours-tens',
'hours-ones',
'minutes-tens',
'minutes-ones',
'seconds-tens',
'seconds-ones'
];
this.buildDigits(currentTime);
getCurrentTime()
const date = new Date();
const time =
hours: date.getHours(),
minutes: date.getMinutes(),
seconds: date.getSeconds(),
;
this.formatValues(time);
return time;
formatValues(time)
Object.keys(time).forEach(key =>
if (key === "ampm") return;
let value = time[key];
if (parseInt(value) < 10)
time[key] = "0" + value;
time[key] = time[key].toString();
);
getDigitProps(digitName)
const type = digitName.split('-')[0];
const position = digitName.split('-')[1];
let positionIndex;
switch (position)
case 'tens':
positionIndex = 0;
break;
case 'ones':
positionIndex = 1;
break;
return type, position, positionIndex ;
buildDigits(time)
this.digits.forEach((digitName) =>
const type, position, positionIndex = this.getDigitProps(digitName);
const selector = `#$type .$position-digit`;
this[digitName] = new Digit(
selector,
value: time[type][positionIndex]
);
);
tick()
const time = this.getCurrentTime();
this.digits.forEach((digitName) =>
const type, positionIndex = this.getDigitProps(digitName);
this[digitName].setValue(time[type][positionIndex]);
);
const clock = new Clock();
setInterval(() =>
clock.tick();
, 1000);
javascript datetime animation dom timer
Here are two simple classes - Digit class maintains digit behaviour - primarily flipping animations, and Clock class handles everything that is related to time data.
I'm going to use this code for job application, please tell me if there are any improvements that could be done and what do you think of it.
Here is the link for working demo: https://abm0.github.io/flipping-clock.
class Digit
constructor( selector, value = 0 )
const digitEl = document.querySelector(selector);
this.flipperEls = digitEl.querySelectorAll('.flipper');
this.prevDigitEls = digitEl.querySelectorAll('.prev .digit');
this.nextDigitEls = digitEl.querySelectorAll('.next .digit');
this.value = value;
this.prevValue = null;
this.renderInitialValue();
setValue(nextValue)
this.prevValue = this.value;
this.value = nextValue;
if (this.value === this.prevValue) return;
this.flip();
renderInitialValue()
const
prevDigitEls,
nextDigitEls,
= this;
[...prevDigitEls, ...nextDigitEls].forEach(el => (el.innerHTML = this.value));
flip()
this.nextDigitEls.forEach(el => el.innerHTML = this.value);
this.flipperEls.forEach(el => el.classList.add('turned'));
setTimeout(() =>
this.prevDigitEls.forEach(el => (el.innerHTML = this.value));
this.flipperEls.forEach((el) =>
el.classList.remove('turned');
);
, 500);
class Clock
constructor(props)
const baseEl = document.querySelector("#clock");
const currentTime = this.getCurrentTime();
this.digits = [
'hours-tens',
'hours-ones',
'minutes-tens',
'minutes-ones',
'seconds-tens',
'seconds-ones'
];
this.buildDigits(currentTime);
getCurrentTime()
const date = new Date();
const time =
hours: date.getHours(),
minutes: date.getMinutes(),
seconds: date.getSeconds(),
;
this.formatValues(time);
return time;
formatValues(time)
Object.keys(time).forEach(key =>
if (key === "ampm") return;
let value = time[key];
if (parseInt(value) < 10)
time[key] = "0" + value;
time[key] = time[key].toString();
);
getDigitProps(digitName)
const type = digitName.split('-')[0];
const position = digitName.split('-')[1];
let positionIndex;
switch (position)
case 'tens':
positionIndex = 0;
break;
case 'ones':
positionIndex = 1;
break;
return type, position, positionIndex ;
buildDigits(time)
this.digits.forEach((digitName) =>
const type, position, positionIndex = this.getDigitProps(digitName);
const selector = `#$type .$position-digit`;
this[digitName] = new Digit(
selector,
value: time[type][positionIndex]
);
);
tick()
const time = this.getCurrentTime();
this.digits.forEach((digitName) =>
const type, positionIndex = this.getDigitProps(digitName);
this[digitName].setValue(time[type][positionIndex]);
);
const clock = new Clock();
setInterval(() =>
clock.tick();
, 1000);
javascript datetime animation dom timer
edited Jul 18 at 16:23
200_success
123k14143399
123k14143399
asked Jul 18 at 13:14
abstractmind
184
184
WithsetInterval( ...., 1000)you'll get your clock to tick once every second. Be aware, however, it's not said it will tick at the beginning of a second. It may tick at the end of each second as well, hence it can be effectively one second late. You may want to set the interval to100to reduce the possible delay in refresh to 1/10 of second.
â CiaPan
Jul 18 at 13:29
add a comment |Â
WithsetInterval( ...., 1000)you'll get your clock to tick once every second. Be aware, however, it's not said it will tick at the beginning of a second. It may tick at the end of each second as well, hence it can be effectively one second late. You may want to set the interval to100to reduce the possible delay in refresh to 1/10 of second.
â CiaPan
Jul 18 at 13:29
With
setInterval( ...., 1000) you'll get your clock to tick once every second. Be aware, however, it's not said it will tick at the beginning of a second. It may tick at the end of each second as well, hence it can be effectively one second late. You may want to set the interval to 100 to reduce the possible delay in refresh to 1/10 of second.â CiaPan
Jul 18 at 13:29
With
setInterval( ...., 1000) you'll get your clock to tick once every second. Be aware, however, it's not said it will tick at the beginning of a second. It may tick at the end of each second as well, hence it can be effectively one second late. You may want to set the interval to 100 to reduce the possible delay in refresh to 1/10 of second.â CiaPan
Jul 18 at 13:29
add a comment |Â
1 Answer
1
active
oldest
votes
up vote
1
down vote
accepted
Review
If you are after a entry level job then your code shows you can code, which is a good start.
As an emploier I would ask.
- Why did you choose to use class syntax over more traditional code styles?
- How long did it take you to write this code?
Bad points
Some points that detract from the code quality
JS
- Clock constructor takes an argument that is not used.
constructor(props) - Unused variable in
Clockconstructorconst baseEl = document.querySelector("#clock"); - Hard coded object is tested for a key it will never have
if (key === "ampm") return; - Clock can only exist as a single instance, but nothing prevents clock being constructed many times.
- Setting element content via
innerHTMLeven though the content has no markup.this.nextDigitEls.forEach(el => el.innerHTML = this.value); - Redundancy suggests you are not sure about type coercion
if (parseInt(value) < 10) { - You have Not used getters and setters yet have the functions
Digit.setValue,Clock.getCurrentTime - The
Clocktakes on aDigits responsibility ?Clock.getDigitProps - Statement with only two outcomes written using
switchnot as aternaryin functionClock.getDigitProps - Lots of redundant code. An example
const prevDigitEls, nextDigitEls = this;followed by the line[...prevDigitEls, ...nextDigitEls]. - Use event listener to get the flip animation end rather than using
setTimeout
CSS & HTML
Its a complete mess, I was going to fix it but it is unusable and needs to be worked from the ground up.
- Bad name for flip animation class
turnedwhich you add to an element to start the animation and remove to reset. - Your code uses ECMAScript6+, yet you include legacy browser CSS, all the legacy CSS is wasted as the clock will not start on those browsers.
- It would be far better to have a container that contains data properties and then build the elements in code. For example HTML only needs
<div class="clock" data-type="flip" data-zone="local" data-format="hh:mm:ss"></div>to create the clock
Rewrite
Its just the code.
Personally I would not have written it as follows because the class syntax forces you to break the rules of encapsulation (No privates) and as its only ever to be a single instance Clock which in reality only needs the tick function which could just be a function.
;(() =>
const digitNames = ['hours-tens', 'hours-ones', 'minutes-tens', 'minutes-ones', 'seconds-tens', 'seconds-ones'];
const pad0 = num => ("" + num).padStart(2, "0");
const time = (d = new Date()) => pad0(d.getHours()) + pad0(d.getMinutes()) + pad0(d.getSeconds());
const elementsText = (elements, text) => for (const el of elements) el.textContent = text
class Digit
constructor(name, timeString)
this.index = digitNames.indexOf(name);
const [type, position] = name.split("-");
const container = document.querySelector(`#$type .$position-digit`);
this.flipperEls = container.querySelectorAll('.flipper');
this.nextDigitEls = container.querySelectorAll('.next .digit');
this.currentDigitEls = container.querySelectorAll('.prev .digit');
this.next = this.val = timeString[this.index];
this.current = this.val;
set time(timeStr)
if (timeStr[this.index] !== this.val)
this.next = this.val = timeStr[this.index];
this.flippers = "turn";
setTimeout(() =>
this.current = this.val;
this.flippers = "turned";
, 500);
set next(value) elementsText(this.nextDigitEls, value)
set current(value) elementsText(this.currentDigitEls, value)
set flippers(action)
for (const el of this.flipperEls)
if (action === "turn") el.classList.add('turned')
else el.classList.remove("turned")
class Clock
constructor()
const timeStr = time();
this.digits = digitNames.map(name => new Digit(name, timeStr));
(this.tick = this.ticker.bind(this))(); // start the clock
ticker() 0) + 1) * 1000 - now;
setTimeout(this.tick, nextSecondIn + (nextSecondIn > 500 ? 0 : 1000));
new Clock();
)();
Thanks for your review! What do you mean by more traditional code styles? Functional? And regarding legacy CSS do you mean that I should have used flexbox/CSS grids?
â abstractmind
Jul 19 at 13:39
"traditional code styles" such as object literalfoo = /* define object */or function statementfunction foo () /* define object */or declarativeObject.create(proto, props),Object.definePropertyetc... or any of many more. CSS? No you had CSS for old browsers that do not support ES6+ so that CSS is of no use.
â Blindman67
Jul 19 at 15:27
add a comment |Â
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
1
down vote
accepted
Review
If you are after a entry level job then your code shows you can code, which is a good start.
As an emploier I would ask.
- Why did you choose to use class syntax over more traditional code styles?
- How long did it take you to write this code?
Bad points
Some points that detract from the code quality
JS
- Clock constructor takes an argument that is not used.
constructor(props) - Unused variable in
Clockconstructorconst baseEl = document.querySelector("#clock"); - Hard coded object is tested for a key it will never have
if (key === "ampm") return; - Clock can only exist as a single instance, but nothing prevents clock being constructed many times.
- Setting element content via
innerHTMLeven though the content has no markup.this.nextDigitEls.forEach(el => el.innerHTML = this.value); - Redundancy suggests you are not sure about type coercion
if (parseInt(value) < 10) { - You have Not used getters and setters yet have the functions
Digit.setValue,Clock.getCurrentTime - The
Clocktakes on aDigits responsibility ?Clock.getDigitProps - Statement with only two outcomes written using
switchnot as aternaryin functionClock.getDigitProps - Lots of redundant code. An example
const prevDigitEls, nextDigitEls = this;followed by the line[...prevDigitEls, ...nextDigitEls]. - Use event listener to get the flip animation end rather than using
setTimeout
CSS & HTML
Its a complete mess, I was going to fix it but it is unusable and needs to be worked from the ground up.
- Bad name for flip animation class
turnedwhich you add to an element to start the animation and remove to reset. - Your code uses ECMAScript6+, yet you include legacy browser CSS, all the legacy CSS is wasted as the clock will not start on those browsers.
- It would be far better to have a container that contains data properties and then build the elements in code. For example HTML only needs
<div class="clock" data-type="flip" data-zone="local" data-format="hh:mm:ss"></div>to create the clock
Rewrite
Its just the code.
Personally I would not have written it as follows because the class syntax forces you to break the rules of encapsulation (No privates) and as its only ever to be a single instance Clock which in reality only needs the tick function which could just be a function.
;(() =>
const digitNames = ['hours-tens', 'hours-ones', 'minutes-tens', 'minutes-ones', 'seconds-tens', 'seconds-ones'];
const pad0 = num => ("" + num).padStart(2, "0");
const time = (d = new Date()) => pad0(d.getHours()) + pad0(d.getMinutes()) + pad0(d.getSeconds());
const elementsText = (elements, text) => for (const el of elements) el.textContent = text
class Digit
constructor(name, timeString)
this.index = digitNames.indexOf(name);
const [type, position] = name.split("-");
const container = document.querySelector(`#$type .$position-digit`);
this.flipperEls = container.querySelectorAll('.flipper');
this.nextDigitEls = container.querySelectorAll('.next .digit');
this.currentDigitEls = container.querySelectorAll('.prev .digit');
this.next = this.val = timeString[this.index];
this.current = this.val;
set time(timeStr)
if (timeStr[this.index] !== this.val)
this.next = this.val = timeStr[this.index];
this.flippers = "turn";
setTimeout(() =>
this.current = this.val;
this.flippers = "turned";
, 500);
set next(value) elementsText(this.nextDigitEls, value)
set current(value) elementsText(this.currentDigitEls, value)
set flippers(action)
for (const el of this.flipperEls)
if (action === "turn") el.classList.add('turned')
else el.classList.remove("turned")
class Clock
constructor()
const timeStr = time();
this.digits = digitNames.map(name => new Digit(name, timeStr));
(this.tick = this.ticker.bind(this))(); // start the clock
ticker() 0) + 1) * 1000 - now;
setTimeout(this.tick, nextSecondIn + (nextSecondIn > 500 ? 0 : 1000));
new Clock();
)();
Thanks for your review! What do you mean by more traditional code styles? Functional? And regarding legacy CSS do you mean that I should have used flexbox/CSS grids?
â abstractmind
Jul 19 at 13:39
"traditional code styles" such as object literalfoo = /* define object */or function statementfunction foo () /* define object */or declarativeObject.create(proto, props),Object.definePropertyetc... or any of many more. CSS? No you had CSS for old browsers that do not support ES6+ so that CSS is of no use.
â Blindman67
Jul 19 at 15:27
add a comment |Â
up vote
1
down vote
accepted
Review
If you are after a entry level job then your code shows you can code, which is a good start.
As an emploier I would ask.
- Why did you choose to use class syntax over more traditional code styles?
- How long did it take you to write this code?
Bad points
Some points that detract from the code quality
JS
- Clock constructor takes an argument that is not used.
constructor(props) - Unused variable in
Clockconstructorconst baseEl = document.querySelector("#clock"); - Hard coded object is tested for a key it will never have
if (key === "ampm") return; - Clock can only exist as a single instance, but nothing prevents clock being constructed many times.
- Setting element content via
innerHTMLeven though the content has no markup.this.nextDigitEls.forEach(el => el.innerHTML = this.value); - Redundancy suggests you are not sure about type coercion
if (parseInt(value) < 10) { - You have Not used getters and setters yet have the functions
Digit.setValue,Clock.getCurrentTime - The
Clocktakes on aDigits responsibility ?Clock.getDigitProps - Statement with only two outcomes written using
switchnot as aternaryin functionClock.getDigitProps - Lots of redundant code. An example
const prevDigitEls, nextDigitEls = this;followed by the line[...prevDigitEls, ...nextDigitEls]. - Use event listener to get the flip animation end rather than using
setTimeout
CSS & HTML
Its a complete mess, I was going to fix it but it is unusable and needs to be worked from the ground up.
- Bad name for flip animation class
turnedwhich you add to an element to start the animation and remove to reset. - Your code uses ECMAScript6+, yet you include legacy browser CSS, all the legacy CSS is wasted as the clock will not start on those browsers.
- It would be far better to have a container that contains data properties and then build the elements in code. For example HTML only needs
<div class="clock" data-type="flip" data-zone="local" data-format="hh:mm:ss"></div>to create the clock
Rewrite
Its just the code.
Personally I would not have written it as follows because the class syntax forces you to break the rules of encapsulation (No privates) and as its only ever to be a single instance Clock which in reality only needs the tick function which could just be a function.
;(() =>
const digitNames = ['hours-tens', 'hours-ones', 'minutes-tens', 'minutes-ones', 'seconds-tens', 'seconds-ones'];
const pad0 = num => ("" + num).padStart(2, "0");
const time = (d = new Date()) => pad0(d.getHours()) + pad0(d.getMinutes()) + pad0(d.getSeconds());
const elementsText = (elements, text) => for (const el of elements) el.textContent = text
class Digit
constructor(name, timeString)
this.index = digitNames.indexOf(name);
const [type, position] = name.split("-");
const container = document.querySelector(`#$type .$position-digit`);
this.flipperEls = container.querySelectorAll('.flipper');
this.nextDigitEls = container.querySelectorAll('.next .digit');
this.currentDigitEls = container.querySelectorAll('.prev .digit');
this.next = this.val = timeString[this.index];
this.current = this.val;
set time(timeStr)
if (timeStr[this.index] !== this.val)
this.next = this.val = timeStr[this.index];
this.flippers = "turn";
setTimeout(() =>
this.current = this.val;
this.flippers = "turned";
, 500);
set next(value) elementsText(this.nextDigitEls, value)
set current(value) elementsText(this.currentDigitEls, value)
set flippers(action)
for (const el of this.flipperEls)
if (action === "turn") el.classList.add('turned')
else el.classList.remove("turned")
class Clock
constructor()
const timeStr = time();
this.digits = digitNames.map(name => new Digit(name, timeStr));
(this.tick = this.ticker.bind(this))(); // start the clock
ticker() 0) + 1) * 1000 - now;
setTimeout(this.tick, nextSecondIn + (nextSecondIn > 500 ? 0 : 1000));
new Clock();
)();
Thanks for your review! What do you mean by more traditional code styles? Functional? And regarding legacy CSS do you mean that I should have used flexbox/CSS grids?
â abstractmind
Jul 19 at 13:39
"traditional code styles" such as object literalfoo = /* define object */or function statementfunction foo () /* define object */or declarativeObject.create(proto, props),Object.definePropertyetc... or any of many more. CSS? No you had CSS for old browsers that do not support ES6+ so that CSS is of no use.
â Blindman67
Jul 19 at 15:27
add a comment |Â
up vote
1
down vote
accepted
up vote
1
down vote
accepted
Review
If you are after a entry level job then your code shows you can code, which is a good start.
As an emploier I would ask.
- Why did you choose to use class syntax over more traditional code styles?
- How long did it take you to write this code?
Bad points
Some points that detract from the code quality
JS
- Clock constructor takes an argument that is not used.
constructor(props) - Unused variable in
Clockconstructorconst baseEl = document.querySelector("#clock"); - Hard coded object is tested for a key it will never have
if (key === "ampm") return; - Clock can only exist as a single instance, but nothing prevents clock being constructed many times.
- Setting element content via
innerHTMLeven though the content has no markup.this.nextDigitEls.forEach(el => el.innerHTML = this.value); - Redundancy suggests you are not sure about type coercion
if (parseInt(value) < 10) { - You have Not used getters and setters yet have the functions
Digit.setValue,Clock.getCurrentTime - The
Clocktakes on aDigits responsibility ?Clock.getDigitProps - Statement with only two outcomes written using
switchnot as aternaryin functionClock.getDigitProps - Lots of redundant code. An example
const prevDigitEls, nextDigitEls = this;followed by the line[...prevDigitEls, ...nextDigitEls]. - Use event listener to get the flip animation end rather than using
setTimeout
CSS & HTML
Its a complete mess, I was going to fix it but it is unusable and needs to be worked from the ground up.
- Bad name for flip animation class
turnedwhich you add to an element to start the animation and remove to reset. - Your code uses ECMAScript6+, yet you include legacy browser CSS, all the legacy CSS is wasted as the clock will not start on those browsers.
- It would be far better to have a container that contains data properties and then build the elements in code. For example HTML only needs
<div class="clock" data-type="flip" data-zone="local" data-format="hh:mm:ss"></div>to create the clock
Rewrite
Its just the code.
Personally I would not have written it as follows because the class syntax forces you to break the rules of encapsulation (No privates) and as its only ever to be a single instance Clock which in reality only needs the tick function which could just be a function.
;(() =>
const digitNames = ['hours-tens', 'hours-ones', 'minutes-tens', 'minutes-ones', 'seconds-tens', 'seconds-ones'];
const pad0 = num => ("" + num).padStart(2, "0");
const time = (d = new Date()) => pad0(d.getHours()) + pad0(d.getMinutes()) + pad0(d.getSeconds());
const elementsText = (elements, text) => for (const el of elements) el.textContent = text
class Digit
constructor(name, timeString)
this.index = digitNames.indexOf(name);
const [type, position] = name.split("-");
const container = document.querySelector(`#$type .$position-digit`);
this.flipperEls = container.querySelectorAll('.flipper');
this.nextDigitEls = container.querySelectorAll('.next .digit');
this.currentDigitEls = container.querySelectorAll('.prev .digit');
this.next = this.val = timeString[this.index];
this.current = this.val;
set time(timeStr)
if (timeStr[this.index] !== this.val)
this.next = this.val = timeStr[this.index];
this.flippers = "turn";
setTimeout(() =>
this.current = this.val;
this.flippers = "turned";
, 500);
set next(value) elementsText(this.nextDigitEls, value)
set current(value) elementsText(this.currentDigitEls, value)
set flippers(action)
for (const el of this.flipperEls)
if (action === "turn") el.classList.add('turned')
else el.classList.remove("turned")
class Clock
constructor()
const timeStr = time();
this.digits = digitNames.map(name => new Digit(name, timeStr));
(this.tick = this.ticker.bind(this))(); // start the clock
ticker() 0) + 1) * 1000 - now;
setTimeout(this.tick, nextSecondIn + (nextSecondIn > 500 ? 0 : 1000));
new Clock();
)();
Review
If you are after a entry level job then your code shows you can code, which is a good start.
As an emploier I would ask.
- Why did you choose to use class syntax over more traditional code styles?
- How long did it take you to write this code?
Bad points
Some points that detract from the code quality
JS
- Clock constructor takes an argument that is not used.
constructor(props) - Unused variable in
Clockconstructorconst baseEl = document.querySelector("#clock"); - Hard coded object is tested for a key it will never have
if (key === "ampm") return; - Clock can only exist as a single instance, but nothing prevents clock being constructed many times.
- Setting element content via
innerHTMLeven though the content has no markup.this.nextDigitEls.forEach(el => el.innerHTML = this.value); - Redundancy suggests you are not sure about type coercion
if (parseInt(value) < 10) { - You have Not used getters and setters yet have the functions
Digit.setValue,Clock.getCurrentTime - The
Clocktakes on aDigits responsibility ?Clock.getDigitProps - Statement with only two outcomes written using
switchnot as aternaryin functionClock.getDigitProps - Lots of redundant code. An example
const prevDigitEls, nextDigitEls = this;followed by the line[...prevDigitEls, ...nextDigitEls]. - Use event listener to get the flip animation end rather than using
setTimeout
CSS & HTML
Its a complete mess, I was going to fix it but it is unusable and needs to be worked from the ground up.
- Bad name for flip animation class
turnedwhich you add to an element to start the animation and remove to reset. - Your code uses ECMAScript6+, yet you include legacy browser CSS, all the legacy CSS is wasted as the clock will not start on those browsers.
- It would be far better to have a container that contains data properties and then build the elements in code. For example HTML only needs
<div class="clock" data-type="flip" data-zone="local" data-format="hh:mm:ss"></div>to create the clock
Rewrite
Its just the code.
Personally I would not have written it as follows because the class syntax forces you to break the rules of encapsulation (No privates) and as its only ever to be a single instance Clock which in reality only needs the tick function which could just be a function.
;(() =>
const digitNames = ['hours-tens', 'hours-ones', 'minutes-tens', 'minutes-ones', 'seconds-tens', 'seconds-ones'];
const pad0 = num => ("" + num).padStart(2, "0");
const time = (d = new Date()) => pad0(d.getHours()) + pad0(d.getMinutes()) + pad0(d.getSeconds());
const elementsText = (elements, text) => for (const el of elements) el.textContent = text
class Digit
constructor(name, timeString)
this.index = digitNames.indexOf(name);
const [type, position] = name.split("-");
const container = document.querySelector(`#$type .$position-digit`);
this.flipperEls = container.querySelectorAll('.flipper');
this.nextDigitEls = container.querySelectorAll('.next .digit');
this.currentDigitEls = container.querySelectorAll('.prev .digit');
this.next = this.val = timeString[this.index];
this.current = this.val;
set time(timeStr)
if (timeStr[this.index] !== this.val)
this.next = this.val = timeStr[this.index];
this.flippers = "turn";
setTimeout(() =>
this.current = this.val;
this.flippers = "turned";
, 500);
set next(value) elementsText(this.nextDigitEls, value)
set current(value) elementsText(this.currentDigitEls, value)
set flippers(action)
for (const el of this.flipperEls)
if (action === "turn") el.classList.add('turned')
else el.classList.remove("turned")
class Clock
constructor()
const timeStr = time();
this.digits = digitNames.map(name => new Digit(name, timeStr));
(this.tick = this.ticker.bind(this))(); // start the clock
ticker() 0) + 1) * 1000 - now;
setTimeout(this.tick, nextSecondIn + (nextSecondIn > 500 ? 0 : 1000));
new Clock();
)();
edited Jul 18 at 22:19
answered Jul 18 at 22:12
Blindman67
5,2281320
5,2281320
Thanks for your review! What do you mean by more traditional code styles? Functional? And regarding legacy CSS do you mean that I should have used flexbox/CSS grids?
â abstractmind
Jul 19 at 13:39
"traditional code styles" such as object literalfoo = /* define object */or function statementfunction foo () /* define object */or declarativeObject.create(proto, props),Object.definePropertyetc... or any of many more. CSS? No you had CSS for old browsers that do not support ES6+ so that CSS is of no use.
â Blindman67
Jul 19 at 15:27
add a comment |Â
Thanks for your review! What do you mean by more traditional code styles? Functional? And regarding legacy CSS do you mean that I should have used flexbox/CSS grids?
â abstractmind
Jul 19 at 13:39
"traditional code styles" such as object literalfoo = /* define object */or function statementfunction foo () /* define object */or declarativeObject.create(proto, props),Object.definePropertyetc... or any of many more. CSS? No you had CSS for old browsers that do not support ES6+ so that CSS is of no use.
â Blindman67
Jul 19 at 15:27
Thanks for your review! What do you mean by more traditional code styles? Functional? And regarding legacy CSS do you mean that I should have used flexbox/CSS grids?
â abstractmind
Jul 19 at 13:39
Thanks for your review! What do you mean by more traditional code styles? Functional? And regarding legacy CSS do you mean that I should have used flexbox/CSS grids?
â abstractmind
Jul 19 at 13:39
"traditional code styles" such as object literal
foo = /* define object */ or function statement function foo () /* define object */ or declarative Object.create(proto, props), Object.defineProperty etc... or any of many more. CSS? No you had CSS for old browsers that do not support ES6+ so that CSS is of no use.â Blindman67
Jul 19 at 15:27
"traditional code styles" such as object literal
foo = /* define object */ or function statement function foo () /* define object */ or declarative Object.create(proto, props), Object.defineProperty etc... or any of many more. CSS? No you had CSS for old browsers that do not support ES6+ so that CSS is of no use.â Blindman67
Jul 19 at 15:27
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f199747%2fjavascript-flipping-clock%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
With
setInterval( ...., 1000)you'll get your clock to tick once every second. Be aware, however, it's not said it will tick at the beginning of a second. It may tick at the end of each second as well, hence it can be effectively one second late. You may want to set the interval to100to reduce the possible delay in refresh to 1/10 of second.â CiaPan
Jul 18 at 13:29