JavaScript flipping clock

The name of the pictureThe name of the pictureThe name of the pictureClash 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);






share|improve this question





















  • 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
















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);






share|improve this question





















  • 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












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);






share|improve this question













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);








share|improve this question












share|improve this question




share|improve this question








edited Jul 18 at 16:23









200_success

123k14143399




123k14143399









asked Jul 18 at 13:14









abstractmind

184




184











  • 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















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










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.



  1. Why did you choose to use class syntax over more traditional code styles?

  2. 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 Clock constructor const 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 innerHTML even 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 Clock takes on a Digit s responsibility ? Clock.getDigitProps

  • Statement with only two outcomes written using switch not as a ternary in function Clock.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 turned which 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();
)();





share|improve this answer























  • 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










Your Answer




StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);








 

draft saved


draft discarded


















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






























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.



  1. Why did you choose to use class syntax over more traditional code styles?

  2. 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 Clock constructor const 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 innerHTML even 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 Clock takes on a Digit s responsibility ? Clock.getDigitProps

  • Statement with only two outcomes written using switch not as a ternary in function Clock.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 turned which 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();
)();





share|improve this answer























  • 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














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.



  1. Why did you choose to use class syntax over more traditional code styles?

  2. 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 Clock constructor const 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 innerHTML even 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 Clock takes on a Digit s responsibility ? Clock.getDigitProps

  • Statement with only two outcomes written using switch not as a ternary in function Clock.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 turned which 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();
)();





share|improve this answer























  • 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












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.



  1. Why did you choose to use class syntax over more traditional code styles?

  2. 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 Clock constructor const 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 innerHTML even 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 Clock takes on a Digit s responsibility ? Clock.getDigitProps

  • Statement with only two outcomes written using switch not as a ternary in function Clock.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 turned which 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();
)();





share|improve this answer















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.



  1. Why did you choose to use class syntax over more traditional code styles?

  2. 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 Clock constructor const 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 innerHTML even 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 Clock takes on a Digit s responsibility ? Clock.getDigitProps

  • Statement with only two outcomes written using switch not as a ternary in function Clock.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 turned which 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();
)();






share|improve this answer















share|improve this answer



share|improve this answer








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 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
















  • 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















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












 

draft saved


draft discarded


























 


draft saved


draft discarded














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













































































Popular posts from this blog

Python Lists

Aion

JavaScript Array Iteration Methods