0

I have a class that supposedly handles generation of a unique ID of uint type. However upon multiple use, the uint value does not seem to increment.

static class UniqueIdGenerator
    {
        public static uint nextUnique;

        public static uint GetNextUnique()
        {
            nextUnique++;
            return nextUnique;
        }
    }

This class is used to assign a unique integer ID whenever it's called on other classes. e.g. both of the sample constructors below have an ID property that's supposed to get its value from the UniqueIdGenerator class

 public Car()
     {
        carNumber = UniqueIdGenerator.GetNextUnique();
     }

public Boat()
     {
        boatNumber = UniqueIdGenerator.GetNextUnique();
     }
Programmer
  • 291
  • 1
  • 3
  • 14
  • 2
    Tip: always use `this.` when referring to instance members and use `_` as a prefix for static members, or use the type-name as a specifier. Otherwise it's impossible to tell from your posted code if `carNumber` is `static` or not. – Dai Mar 05 '21 at 17:52
  • 2
    You should use `return Interlocked.Increment( ref staticVar )` instead of using the `++` operator - otherwise you'll run into concurrency issues. – Dai Mar 05 '21 at 17:54
  • @Dai that is if there are multiple threads. Could happen if it's a web application. nextUnique is not initialized to zero. – Tarik Mar 05 '21 at 18:21

1 Answers1

0

The code posted should work, as far as I can tell. It does come with troubles though.

The first trouble is thread safety. Two threads can attempt to increment the same value and result in having the same ID. To fix this use Interlocked.Increment(ref nextUnique)

The second potential trouble is a security one - incremental IDs can lead to web APIs that are easy to scrape. Zoom had a similar problem last year with their meeting IDs if I remember correctly.

If you need IDs using a GUID solves both the problems and it comes with the added benefit of not having to worry too much about IDs colliding in a distributed system:

public class MyBusinessObject
{
  public Guid ID {get; private set;} = Guid.NewGuid();
}

The private set is to enable deserializing, while prohibiting directly setting the value in code.

I realize this is too much for what you probably see as a very simple question, but ignoring the hidden complexities can lead to getting hacked down the road.

Cheers!

Sten Petrov
  • 10,943
  • 1
  • 41
  • 61
  • Guids are pretty random and certainly good in distributed systems, but they are not meant to be secure against real guessing attacks. See for example https://stackoverflow.com/a/4517620/4626765 for more details. – Mikael Suokas Mar 05 '21 at 20:48
  • @MikaelSuokas My proposal is to use them as an ID, not a security token. On Windows they are actually random with the current implementation (v4), but there are no public claims about the strength of the quality of the random generator. – Sten Petrov Mar 07 '21 at 00:17