2
\$\begingroup\$

I'm new to React.js and this code came from 30 Days of React - Day 7. However the given code example wasn't working so I rewrote it myself. My biggest issue here is how to make the component work without function getTime() using both setState and return. Any other suggestions are appreciated.

class Clock extends React.Component {
    constructor(props) {
        super(props);
        this.state = {currentTime: this.getTime()};
    }

    getTime() {
        const currentTime = new Date(),
        hours = currentTime.getHours(),
        minutes = currentTime.getMinutes(),
        seconds = currentTime.getSeconds(),
        ampm = hours >= 12 ? 'pm' : 'am';
        this.setState({currentTime: {hours, minutes, seconds, ampm}});
        return {hours, minutes, seconds, ampm};
    }

    componentDidMount() {
        const intervalId = setInterval(this.getTime.bind(this), 1000);
        this.setState({intervalId: intervalId}); 
    }

    componentWillDisMount() {
        clearInterval(this.state.intervalId);
    }

    render() {
        const {hours, minutes, seconds, ampm} = this.state.currentTime;
        return (
            <div className="clock">
                {hours == 0 ? 12 : (hours > 12) ? hours - 12 : hours}:
                {minutes > 9 ? minutes : `0${minutes}`}:
                {seconds > 9 ? seconds : `0${seconds}`} {ampm}
            </div>
        )   
    }
}

ReactDOM.render(<Clock />, document.querySelector("#app"));
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/15.1.0/react.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/15.1.0/react-dom.min.js"></script>

<body>
  <div id="app"></div>
</body>

\$\endgroup\$
0
\$\begingroup\$

If it's assigning to the state, you don't need to assign it at the beginning.

this.setState({currentTime: {hours, minutes, seconds, ampm}});  // you don't need to return at all.

You can replace this with some value that indicates that it hasn't loaded yet, which also has the benefit of not showing up for a page crawler.

Rewrite

class Clock extends React.Component {
    constructor(props) {
       super(props);
       this.state = {currentTime: {hours:0, minutes:0, seconds: 0, ampm: 'N/A'}};
    }
    
    getTime() {
        const currentTime = new Date(),
            hours = currentTime.getHours(),
            minutes = currentTime.getMinutes(),
            seconds = currentTime.getSeconds(),
            ampm = hours >= 12 ? 'pm' : 'am';
            this.setState({currentTime: {hours, minutes, seconds, ampm}});
        }
    
        componentDidMount() {
            this.getTime();
            const intervalId = setInterval(this.getTime.bind(this), 1000);
            this.setState({intervalId: intervalId});
        }
    
        componentWillDisMount() {
            clearInterval(this.state.intervalId);
        }
    
        render() {
            const {hours, minutes, seconds, ampm} = this.state.currentTime;
            return (
                <div className="clock">
                    {hours == 0 ? 12 : (hours > 12) ? hours - 12 : hours}:
                    {minutes > 9 ? minutes : `0${minutes}`}:
                    {seconds > 9 ? seconds : `0${seconds}`} {ampm}
                </div>
            )   
        }
    }
    
    ReactDOM.render(<Clock/>, document.querySelector("#app"));
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/15.1.0/react.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/15.1.0/react-dom.min.js"></script>

<body>
  <div id="app"></div>
</body>

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

In think you can improve the render by following the React philosofy of writing reusable components.

If you look closer at your code:

render() {
    const {hours, minutes, seconds, ampm} = this.state.currentTime;
    return (
        <div className="clock">
            {hours == 0 ? 12 : (hours > 12) ? hours - 12 : hours}:
            {minutes > 9 ? minutes : `0${minutes}`}:
            {seconds > 9 ? seconds : `0${seconds}`} {ampm}
        </div>
    )   
}

Here you have duplication in printing a digit. You have it with minutes and seconds, but I think hours should be printed the same way too.

You can introduce a display component as a plain function:

const PrintDigit = ({ digit }) =>
    (<React.Fragment>{digit < 9 && '0'}{digit}</React.Fragment>)

The function have props as an input parameter, that here I spread with a variable digit to document the only props expected from the component.

So your render() method could be rewritten:

render() {
    const {hours, minutes, seconds, ampm} = this.state.currentTime;
    return (
        <div className="clock">
            <PrintDigit digit={hours === 0 ? 12 : hours % 12} />:
            <PrintDigit digit={minutes} />:
            <PrintDigit digit={seconds} /> {ampm}
        </div>
    )   
}

Is not that big improvement, but as your code is an exercise to learn React, I think is good to start thinking in that way.

The functional component could stay just in the same file of your main component or you can put it in a different file and then import where you need it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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