Code Review Asked by nax-p5 on October 27, 2021
I’m learning OOP and the first exercise of the course was to made a Stopwatch class. I did it really fast and decided to make an interface for this little app.
I created the interface. Basically I want to know if the aproach that I used is fine, if I needed to create another class to control the ui, etc. I’m really lost with this OOP-UI integration thing.
The app is available in https://nacho-p5.github.io/
Here’s the code:
Repo: https://github.com/nacho-p5/nacho-p5.github.io
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<meta http-equiv="X-UA-Compatible" content="ie=edge">
<link rel="stylesheet" href="normalize.css">
<link href="https://fonts.googleapis.com/css2?family=Lato:wght@100;300;700&display=swap" rel="stylesheet">
<link rel="stylesheet" href="style.css">
<title>Online Stopwatch</title>
</head>
<body>
<div id="sw">
<span id="sw-screen">0.00</span>
<div id="buttons">
<button type="submit" id="btnStart" class="btn">Start</button>
<button type="submit" id="btnStop" class="btn">Stop</button type="submit">
<button type="submit" id="btnReset" class="btn">Reset</button type="submit">
</div>
</div>
</body>
<script src="stopwatch.js"></script>
</html>
// SW class
function Stopwatch() {
let counter = 0;
let intervalID = 0;
const screen = document.getElementById('sw-screen');
const addMiliseconds = () => {
counter += 0.01
updateScreen();
};
const updateScreen = () => {
screen.innerHTML = counter.toFixed(2);
}
// start()
this.start = function() {
if (intervalID != 0) {
throw new Error('Stopwatch has already started.')
}
intervalID = setInterval(addMiliseconds, 10);
};
// stop()
this.stop = function() {
if (intervalID == 0) {
throw new Error('Stopwatch has already stoped.')
}
clearInterval(intervalID);
intervalID = 0;
};
// reset()
this.reset = () => {
if (intervalID != 0) {
this.stop();
};
counter = 0;
updateScreen();
};
// duration()
Object.defineProperty(this, 'duration', {
get: function() {
console.log(intervalID);
return counter
}
});
};
// UI class
function UI(arrayOfbuttons) {
this.toggleClass = function(elem) {
arrayOfbuttons.forEach(e => e.classList.remove('activeBtn'))
elem.classList.add('activeBtn');
};
this.removeClass = function() {
arrayOfbuttons.forEach(e => e.classList.remove('activeBtn'))
};
};
// HTML components
const screen = document.getElementById('sw-screen');
const btnStart = document.getElementById('btnStart');
const btnStop = document.getElementById('btnStop');
const btnReset = document.getElementById('btnReset');
// Initialize classes
const sw = new Stopwatch();
const ui = new UI([btnStart, btnStop]);
// Add event listeners
btnStart.addEventListener('click', function() {
sw.start();
ui.toggleClass(this);
});
btnStop.addEventListener('click', function() {
sw.stop();
ui.toggleClass(this);
});
btnReset.addEventListener('click', function() {
sw.reset();
ui.removeClass();
});
First of all, Sam Onela and konijin, many thanks for your responses. They gave me cool start points to continue learning. The proposal of code from konijin was great, I learned a lot reading that.
I made the following changes. This was extracted from your comments:
Also, inspired on konijin's solution I refactored the code using the observer pattern. I had two goals doing that:
What do you think about?
class Stopwatch {
_timer = 0;
isRunning = false;
startTime = 0;
elapsedTime = 0
observers = []
get timer() {
return this._timer
}
set timer(val) {
this._timer = val
this.notifyController(val)
}
registerObserver(observer) {
this.observers.push(observer);
};
notifyController(val) {
this.observers.forEach(observer => {observer.update(val)})
}
updateTime() {
const newTime = Date.now() - this.startTime + this.elapsedTime;
this.timer = newTime;
};
start() {
if (!this.isRunning) {
this.isRunning = true;
this.startTime = Date.now();
this.setIntervalID = setInterval(this.updateTime.bind(this), 100);
};
};
stop() {
if (this.isRunning) {
clearInterval(this.setIntervalID);
this.isRunning = false;
this.elapsedTime = this._timer;
};
};
reset() {
clearInterval(this.setIntervalID);
this.isRunning = false
this.elapsedTime = 0;
this.startTime = 0;
this.timer = 0;
};
};
class UI {
constructor(displayID, btnStartID, btnStopID, btnResetID) {
// HTML Components
this.buttons = {
start: document.getElementById(btnStartID),
stop: document.getElementById(btnStopID),
reset: document.getElementById(btnResetID)
},
this.display = document.getElementById(displayID)
};
resetAllButtonsStyle() {
Object.values(this.buttons).forEach(e => e.classList.remove('activeBtn'))
};
showButtonAsActive(btn) {
this.resetAllButtonsStyle();
btn.classList.add('activeBtn')
};
updateDisplay(value) {
this.display.innerText = value;
};
}
class Controller {
constructor(sw, ui) {
this.sw = sw;
this.ui = ui;
// Add event listeners
this.ui.buttons.start.addEventListener('click', function() {
sw.start();
ui.showButtonAsActive(this);
});
this.ui.buttons.stop.addEventListener('click', function() {
if (sw.isRunning) {
sw.stop();
ui.showButtonAsActive(this);
};
});
this.ui.buttons.reset.addEventListener('click', function() {
sw.reset();
ui.resetAllButtonsStyle();
});
}
update(val) {
ui.updateDisplay((val/1000).toFixed(3))
}
}
// Initialize classes
const ui = new UI('sw-display', 'btnStart', 'btnStop', 'btnReset');
const sw = new Stopwatch();
const controller = new Controller(sw, ui);
// Register controller in sw
sw.registerObserver(controller);
////////////////////////
````
Answered by nax-p5 on October 27, 2021
From a short review;
You cannot trust setInterval(addMiliseconds, 10);
to actually call every 10 milliseconds, JS will only try to honour that and it will not try very hard, your time will always be off, which is shame for a timer ;) See this link for more detail. In essence you need to keep track when the time started and then do something like var delta = Date.now() - start; // milliseconds elapsed since start
You are styling your buttons with a btn
class, since there are no other buttons you might as well style the button
tag itself
I am not a big fan of putting <script>
tags after the <body>
, I would just put the script in the header and wait for the DOM Content Loaded event to trigger.
Other than that, your HTML looks great
screen
is an unfortunate name for the passed time field
Only throw
when you are planning to catch, otherwise you create a bad user experience
This is my counter proposal;
// Initialize classes
const ui = new UI();
const sw = new Stopwatch();
const controller = new Controller(ui, sw);
// SW class
function Stopwatch() {
this.counter = 0;
this.since = 0;
this.intervalID; //Undefined is fine to start with
//stopwatches should not know about the DOM
//const screen = document.getElementById('sw-screen');
this.updateTimer = function updateTimer(){
this.counter = (Date.now() - this.since) / 1000;
};
this.start = function start() {
if (!this.intervalID) {
this.intervalID = setInterval(controller.tick, 10);
this.since = Date.now();
}
};
this.stop = function stop() {
if (this.intervalID) {
clearInterval(this.intervalID);
this.intervalID = 0;
}
};
this.reset = function reset(){
if (this.intervalID != 0) {
this.stop();
this.counter = 0;
};
};
Object.defineProperty(this, 'duration', {
get: function() {
console.log(intervalID);
return counter
}
});
};
// UI class
function UI() {
// HTML components
this.buttons = {
start: document.getElementById('start'),
stop: document.getElementById('stop'),
reset: document.getElementById('reset')
};
this.display = document.getElementById('display');
this.updateDisplay = function(sw){
//If innerText works, then use that
this.display.innerText = sw.counter.toFixed(2);
}
this.showButtonAsActive = function showButtonAsActive(b) {
this.resetAllButtons();
b.classList.add('activeBtn');
};
this.resetAllButtons = function resetAllButtons() {
Object.values(this.buttons).forEach(e => e.classList.remove('activeBtn'))
};
};
function Controller(ui, sw){
// Add event listeners
ui.buttons.start.addEventListener('click', function() {
sw.start();
ui.showButtonAsActive(this);
});
ui.buttons.stop.addEventListener('click', function() {
sw.stop();
ui.showButtonAsActive(this);
});
ui.buttons.reset.addEventListener('click', function() {
sw.reset();
ui.resetAllButtons();
ui.updateDisplay(sw);
});
this.tick = function timerTick(){
sw.updateTimer();
ui.updateDisplay(sw)
}
}
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<meta http-equiv="X-UA-Compatible" content="ie=edge">
<link rel="stylesheet" href="normalize.css">
<link href="https://fonts.googleapis.com/css2?family=Lato:wght@100;300;700&display=swap" rel="stylesheet">
<link rel="stylesheet" href="style.css">
<title>Online Stopwatch</title>
</head>
<body>
<div id="sw">
<span id="display">0.00</span>
<div id="buttons">
<button type="submit" id="start">Start</button>
<button type="submit" id="stop">Stop</button>
<button type="submit" id="reset">Reset</button>
</div>
</div>
</body>
</html>
Answered by konijn on October 27, 2021
Welcome to Code Review! Hopefully you find the feedback here and in other posts useful!
The code makes good use of const
for items that shouldn't be re-assigned (e.g. screen
, btnStart
, screen
, etc.) and let
for re-assignable values (e.g. counter
, intervalID
).
This code uses some ES6 features like arrow functions - thus it could use the class syntax. Until then, each instance of StopWatch
has methods defined using this.[method]
. This works but isn't as efficient as using the prototype. Such a change might likely require changing local variables to instance variables (e.g. screen
=> this.screen
).
The naming of methods on UI
could be more appropriate - e.g. toggleClass
would be more appropriate as toggleActiveClass
because it toggles the activeBtn
class, and removeClass
would be more appropriate as removeActiveClass
for the same reason.
I see that the Error is thrown when clicking the button with text Start twice, which makes sense, but perhaps the click event handler should only call the function sw.start
(as well as ui.toggleClass(this)
) if intervalID === 0
- if that is a case then perhaps a public method to determine if the Stopwatch is started is necessary.
I noticed the HTML for the buttons labeled Stop and Reset have attributes on the close tag:
<button type="submit" id="btnStop" class="btn">Stop</button type="submit"> <button type="submit" id="btnReset" class="btn">Reset</button type="submit">
Setting attributes on the closing tags will either be ignored by the browser, or could lead to an error - see answers to this SO question for more of an explanation.
Answered by Sᴀᴍ Onᴇᴌᴀ on October 27, 2021
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP