8
\$\begingroup\$

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

index.html

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

stopwatch.js

// 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();
});
\$\endgroup\$

3 Answers 3

Reset to default
1
\$\begingroup\$

Welcome to Code Review! Hopefully you find the feedback here and in other posts useful!

Good things

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

Suggestions

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.

\$\endgroup\$
1
\$\begingroup\$

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>

\$\endgroup\$
1
\$\begingroup\$

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:

  • Use ES6 class syntax
  • Adds methods to prototype rather than instance
  • Use appropiate names in vars, attrs and methods
  • Replace the error handler logic when start/stop are clicked twice
  • Remove attributes in closing HTML tags - This was a typo
  • Change time tracker logic. Don't count time based on setInterval iterations
  • Don't put the script tag after body - Changed using defer
  • Only throw when you are planning to catch
  • Stopwatches should not know about the DOM - changed

Also, inspired on konijin's solution I refactored the code using the observer pattern. I had two goals doing that:

  1. Implement the observer pattern
  2. One class - One thing: SW class must be fully functional without a ui, UI class only changes things in DOM and controller class take the decisions

What do you think about?

stopwatch.js


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

////////////////////////


````
\$\endgroup\$
6
  • 1
    \$\begingroup\$ This might be better as a follow up question that links to the original question. \$\endgroup\$
    – pacmaninbw ♦
    Commented Jul 26, 2020 at 13:49
  • 1
    \$\begingroup\$ Also, just saying, an upvote would be nice if you liked our answers ;) \$\endgroup\$
    – konijn
    Commented Jul 27, 2020 at 10:03
  • \$\begingroup\$ Sure konijin! I'm new to this site and don't really know how it works ;) @pacmaninbw follow up question = new question with some kind of link to this? how i can do that? sorry for the noob questions but i'm really new to this site \$\endgroup\$
    – nabla-f
    Commented Jul 27, 2020 at 15:11
  • \$\begingroup\$ This link is a follow up question I asked. To create a link enter the text you want as a link, select that text and use the link icon in the mark down editor for the question (the link icon is 2 links in a chain). You can also just hard code the URL of the original question. \$\endgroup\$
    – pacmaninbw ♦
    Commented Jul 27, 2020 at 16:18
  • \$\begingroup\$ Did you create the question with this unregistered account? if so, you can click the Contact link at the bottom of the page and request the accounts be merged. Also, if you haven't already read it, please read What should I do when someone answers my question?. \$\endgroup\$
    – Sᴀᴍ Onᴇᴌᴀ ♦
    Commented Jul 27, 2020 at 16:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.