Safe way of creating unique IDs

Question

Is the code below safe to create unique ids for objects?

I will create new yoga classes and need to have a unique id for each class. I made this small piece of code to generate the id numbers.

Regarding specifically the uniqueIdCreatorHandler method, would this solution incur in bad performance, or is it just far from elegant?

what would you say?

CodeSandBox here

The code is also presented as per below:

import React from "react";
export default class App extends React.Component {
 state = {
 id: "",
 ids: [{ id: 7 }, { id: 14 }]
 };
 uniqueIdCreatorHandler = () => {
 let ids = [...this.state.ids];
 let highestId = 0;
 if (ids.length > 0) {
 highestId = ids
 .map(value => {
 return value.id;
 })
 .reduce((a, b) => {
 return Math.max(a, b);
 });
 }
 let newId = highestId + 1;
 ids.push({ id: newId });
 this.setState({ ids: ids });
 };
 idDeleterHanlder = currIndex => {
 let ids = this.state.ids;
 ids.splice(currIndex, 1);
 this.setState({ ids: ids });
 };
 render() {
 let ids = this.state.ids.map((id, index) => {
 return (
 <p onClick={() => this.idDeleterHanlder(index)} key={id.id}>
 id:{id.id}
 </p>
 );
 });
 return (
 <div className="App">
 <button onClick={this.uniqueIdCreatorHandler}>Push new id</button>
 <p>all ids below:</p>
 {ids}
 </div>
 );
 }
}

• konijn: If you delete the last index, and push a new one, it will re-use the same index. I would just keep the last created index as part of the state. The code will be much more elegant.

Answer 1

I've noticed you have a common bug that often occurs in Javascript code called a race condition.

If uniqueIdCreatorHandler is called again before a previous call finishes there is a race condition created because state.ids is replaced at the end of the call.

If both calls happen at the same time they each generate a unique copy of the original state.ids then overwrite state this.setState({ ids: ids });. This causes the first change to state.ids to be forgotten.

This is a direct result caused by replacing state. this.setState({ ids: ids });

As others have recommend I would simply keep a increment in state for new ids.

Note: you can easily demonstrate this in your code by adding and calling this method

double = () => {
 this.uniqueIdCreatorHandler()
 this.uniqueIdCreatorHandler()
}

Other comments There are small improvements you could make to your code like spreading the mapped ids into Math.max.

I wouldn't worry too much about Javascript performance, focus on readability. Make variable names more verbose and add doc blocks to methods.

One thing I like to see (which you have done) is the embrace of array methods. Although they may not be necessary here they are a strong indicator of comprehension.

Keep it up!

• Arp: Thanks so much for shedding some light into this humble piece of code. Regarding the asynchronous nature of Js I guess that I should somehow implement prevState arg once I set my state, correct? like: this.setState(prevState => ({id: prevState...do something})
• Cody Wikman: For your use case you "shouldn't" run into any situations where pushing to the ids doesn't work. I would say it's something you should be aware of as a potential problem. From what I understand this is because React batches updates. So you would want to merge all of the changes into a single setState() or use Promises. but that is probably over engineering at this point