0
\$\begingroup\$

There are systems that when you duplicate an asset, the new assets are created with an index, so that the name do not collide. For instance, if I have an asset called House, and I press "Duplicate", a new asset will be created called House (1), and if I press again, it will be called House (2)

I wanted to make a system to duplicate that behavior, but I'm not sure if I am making it too complicated, or faulty. I'm also scared of infinite loops.

How would you improve this?

    public class NameGenerator
    {
        private readonly string baseName;
        
        private readonly Func<string, bool> isValid;
        
        public NameGenerator(string baseName, Func<string, bool> isValid)
        {
            this.baseName = baseName;
            this.isValid = isValid;
        }

        public string Generate()
        {
            if (isValid(baseName)) return baseName;

            for (var i = 0; ; i++)
            {
                var candidate = $"{baseName} ({i})";
                if (isValid(candidate)) return candidate;
            }
        }
    }
```
\$\endgroup\$
4
  • 3
    \$\begingroup\$ Hi @EnriqueMorenoTent - isValid - is more like isUnused or isUnique or isAvailable ?? Validity and Availability are two separate orthogonal concepts ... (for instance does putting (1) on the end still ensure the asset name is valid - even if it does make it unique / available. \$\endgroup\$
    – Mr R
    Apr 10 at 4:00
  • \$\begingroup\$ You have a point. I guess IsUnused would express better its function. Thanks! \$\endgroup\$ Apr 10 at 10:37
  • 1
    \$\begingroup\$ If you need to guarantee unique names then generating new GUIDs as suffixes would make your helper a lot more simpler. \$\endgroup\$ Apr 12 at 6:38
  • \$\begingroup\$ But I need the names to remain readable \$\endgroup\$ Apr 12 at 9:46
1
\$\begingroup\$
  1. Potential infinite loop
    If the isValid method is faulty, i.e always returns false, we'd have an infinite loop on our hands. How do we fix it? I'd fix it in coding in some (configurable but not necessary) upper limit, say 255, so if we tried 255 times, and it still fails, just throw an exception. This could either be done using a public property with a default value, or an optional constructor parameter, then all you'd need to do is check whether i is less than that upper bound.

  2. Naming, specifically underscores
    Technically, there's nothing wrong with your naming right now, as the Microsoft Naming Guidelines is outdated (from 2008) and not complete at all. And this Stack Overflow Q&A states that you shouldn't use underscores, and instead use this to differentiate between members and parameters (as you are right now). But even that is from 2012, and looking at some of Microsoft's own code, like this line in the ASP.NET Core repository shows that even they use an underscore prefix.

Resulting code:

public class NameGenerator
{
    private readonly string _baseName;

    private readonly Func<string, bool> _isValid;

    public int MaxAttempts {get; set;} = 255;

    public NameGenerator(string baseName, Func<string, bool> isValid)
    {
        _baseName = baseName;
        _isValid = isValid;
    }

    public string Generate()
    {
        // This is just a personal preference, keep it as one line if you want
        if (_isValid(_baseName))
            return _baseName;

        for (var i = 0; i < MaxAttempts; i++)
        {
            var candidate = $"{_baseName} ({i})";
            if (_isValid(candidate))
                return candidate;
        }
    }
}

Instead of a property to set the max attempts, you could also use an optional constructor parameter, like this:

public class NameGenerator
{
    // ...
    private readonly int _maxAttempts;

    public NameGenerator(string baseName, Func<string, bool> isValid, int maxAttempts = 255)
    {
        // ...
        _maxAttempts = maxAttempts;
    }

    // ...
}
\$\endgroup\$
2
  • \$\begingroup\$ I didn't think about the case of a faulty isValid method. I agree that capping it is necessary. I also did not consider allowing the user to define the cap. I guess I could make two constructors. One if the user wants to set the cap, and another one if the user wants to use the default cap. Thanks for the feedback. \$\endgroup\$ Apr 12 at 9:50
  • \$\begingroup\$ No need for 2 constructors when one does the trick, if you use the default value for the maxAttempts parameter you can call the one constructor 2 ways. Either new NameGenerator("someName", MyIsValid) or NameGenerator("someName", MyIsValid, 10) without needing to declare multiple constructors \$\endgroup\$
    – MindSwipe
    Apr 12 at 10:19
0
\$\begingroup\$

If you want to take it a bit further and making it a generic unique name / id generator that can be used for anything, we would need to analyse it a bit more.

One important aspect is to reserve the unique name you just generated so it cannot be used by a parallel thread/process as well which is quite likely if both look at what was the last name and increase its number.

Another aspect is to make it generic, we should be able to add a number to a name or get an Int32 id and look for the next available number or adding a Guid to something what-ever comes to our mind. Obviously for making something generic Generics come to my mind.

And I would restructure the class in a way that the base name is not injected in the constructor but passed as parameter to method Generate. This way you can reuse the same instance for 100.000s of names single-threaded or in parallel - if the logic is the same, you only need a single name generator.

That all said, I would identify the parts we need (I define them as interfaces instead of lambdas, but of course the implementing class may define a constructor that takes lambdas to implement it):

  • A value provider which provides the number or other object to distinguish the base name and make it unique for which I didn't invent any special interface and just used an IEnumerable<T>.
  • A unique name ensurer which I implemented as two interfaces called IUniqueNameEnsurer, one just telling "yes, this value is unique" and the other "yes, this value is unique and here is the handle to it" (what ever that is, e.g. a FileStream or a reference to the generated record in the database etc.).
  • A unique name assembler which is responsible to fit the base name and the distinguisher value together and which I named IUniqueNameAssembler that has two methods, one that takes only the base name (for the first object) and one that takes the base name as well as a distinguisher value from the value provider above.

Here their definitions:

public interface IUniqueNameEnsurer<TResultValue> {

    bool IsUnique(TResultValue uniqueName);

}

public interface IUniqueNameEnsurer<TResultValue, TReservation> : IUniqueNameEnsurer<TResultValue> {

    bool IsUnique(TResultValue uniqueName, [NotNullWhen(true)] out TReservation reservation);

}

and

public interface IUniqueNameAssembler<TBaseValue, TDistinguisherValue, TResultValue> {

    TResultValue GenerateUniqueName(TBaseValue baseName);

    TResultValue GenerateUniqueName(TBaseValue baseName, TDistinguisherValue distinguisherValue);

}

Then (optional) we could define some default implementations that map lambda functions to the according interface methods:

public class UniqueNameEnsurer<TResultValue> : IUniqueNameEnsurer<TResultValue> {

    // Constructors

    public UniqueNameEnsurer(Func<TResultValue, bool> uniqueNamePredicate) {
        if (uniqueNamePredicate is null) throw new ArgumentNullException(nameof(uniqueNamePredicate));
        UniqueNamePredicate = uniqueNamePredicate;
    }

    // Public Methods

    public Boolean IsUnique(TResultValue uniqueName) {
        return UniqueNamePredicate.Invoke(uniqueName);
    }

    // Private Properties

    private Func<TResultValue, bool> UniqueNamePredicate { get; }

}

public class UniqueNameEnsurer<TResultValue, TReservation> : UniqueNameEnsurer<TResultValue>, IUniqueNameEnsurer<TResultValue, TReservation> {

    // Constructors

    public UniqueNameEnsurer(Func<TResultValue, bool> uniqueNamePredicate, Func<TResultValue, TReservation?> tryCreateReservationFunction)
            : base(uniqueNamePredicate) {
        if (tryCreateReservationFunction is null) throw new ArgumentNullException(nameof(tryCreateReservationFunction));
        TryCreateReservationFunction = tryCreateReservationFunction;
    }

    // Public Methods

    public Boolean IsUnique(TResultValue uniqueName, [NotNullWhen(true)] out TReservation reservation) {
        reservation = TryCreateReservationFunction.Invoke(uniqueName)!;
        return (reservation is not null);
    }

    // Private Properties

    private Func<TResultValue, TReservation?> TryCreateReservationFunction { get; }

}

and

public class UniqueNameAssembler<TBaseValue, TDistinguisherValue, TResultValue> : IUniqueNameAssembler<TBaseValue, TDistinguisherValue, TResultValue> {

    // Constructors

    public UniqueNameAssembler(Func<TBaseValue, TResultValue> baseNameConverterFunction, Func<TBaseValue, TDistinguisherValue, TResultValue> assemblerFunction) {
        if (baseNameConverterFunction is null) throw new ArgumentNullException(nameof(baseNameConverterFunction));
        if (assemblerFunction is null) throw new ArgumentNullException(nameof(assemblerFunction));
        BaseNameConverterFunction = baseNameConverterFunction;
        AssemblerFunction = assemblerFunction;
    }

    // Public Methods

    public TResultValue GenerateUniqueName(TBaseValue baseName) {
        if (baseName is null) throw new ArgumentNullException(nameof(baseName));
        return BaseNameConverterFunction.Invoke(baseName);
    }

    public TResultValue GenerateUniqueName(TBaseValue baseName, TDistinguisherValue distinguisherValue) {
        if (baseName is null) throw new ArgumentNullException(nameof(baseName));
        return AssemblerFunction.Invoke(baseName, distinguisherValue);
    }

    // Private Properties

    private Func<TBaseValue, TResultValue> BaseNameConverterFunction { get; }

    private Func<TBaseValue, TDistinguisherValue, TResultValue> AssemblerFunction { get; }

}

And as last of the framework classes we have the UniqueNameGenerator in two versions, with or without support for reservations:

(without support for reservations)

public class UniqueNameGenerator<TBaseValue, TDistinguisherValue, TResultValue> {

    // Constructors

    public UniqueNameGenerator(IUniqueNameEnsurer<TResultValue> uniqueNameEnsurer, IEnumerable<TDistinguisherValue> distinguifier, IUniqueNameAssembler<TBaseValue, TDistinguisherValue, TResultValue> valueAssembler) {
        if (uniqueNameEnsurer is null) throw new ArgumentNullException(nameof(uniqueNameEnsurer));
        if (distinguifier is null) throw new ArgumentNullException(nameof(distinguifier));
        if (valueAssembler is null) throw new ArgumentNullException(nameof(valueAssembler));
        UniqueNameEnsurer = uniqueNameEnsurer;
        Distinguifier = distinguifier;
        ValueAssembler = valueAssembler;
    }

    // Public Properties

    public IUniqueNameEnsurer<TResultValue> UniqueNameEnsurer { get; }

    public IEnumerable<TDistinguisherValue> Distinguifier { get; }

    public IUniqueNameAssembler<TBaseValue, TDistinguisherValue, TResultValue> ValueAssembler { get; }

    // Public Methods

    public TResultValue GenerateUniqueName(TBaseValue baseName) {
        if (baseName is null) throw new ArgumentNullException(nameof(baseName));
        // If the base name is unique, return it
        var valueAssembler = ValueAssembler;
        var resultName = valueAssembler.GenerateUniqueName(baseName);
        var uniqueNameEnsurer = UniqueNameEnsurer;
        if (uniqueNameEnsurer.IsUnique(resultName)) return resultName;
        // Otherwise ask for distinguisher values
        var distinguifier = Distinguifier.GetEnumerator();
        while (distinguifier.MoveNext()) {
            resultName = valueAssembler.GenerateUniqueName(baseName, distinguifier.Current);
            if (uniqueNameEnsurer.IsUnique(resultName)) return resultName;
        }
        // If all values are used, throw an exception
        throw new InvalidOperationException("Distinguifier values exceeded!");
    }

}

(with support for reservations)

public class UniqueNameGenerator<TBaseValue, TDistinguisherValue, TResultValue, TReservation> 
    : UniqueNameGenerator<TBaseValue, TDistinguisherValue, TResultValue> {

    // Constructors

    public UniqueNameGenerator(IUniqueNameEnsurer<TResultValue, TReservation> uniqueNameEnsurer, IEnumerable<TDistinguisherValue> distinguifier, IUniqueNameAssembler<TBaseValue, TDistinguisherValue, TResultValue> valueAssembler)
            : base(uniqueNameEnsurer, distinguifier, valueAssembler) {
        UniqueNameEnsurer = uniqueNameEnsurer;
    }

    // Public Properties

    public new IUniqueNameEnsurer<TResultValue, TReservation> UniqueNameEnsurer { get; }

    // Public Methods

    public TResultValue GenerateUniqueName(TBaseValue baseName, out TReservation reservation) {
        if (baseName is null) throw new ArgumentNullException(nameof(baseName));
        // If the base name is unique, return it
        var valueAssembler = ValueAssembler;
        var resultName = valueAssembler.GenerateUniqueName(baseName);
        var uniqueNameEnsurer = UniqueNameEnsurer;
        if (uniqueNameEnsurer.IsUnique(resultName, out reservation)) return resultName;
        // Otherwise ask for distinguisher values
        IEnumerator<TDistinguisherValue> distinguifier = Distinguifier.GetEnumerator();
        while (distinguifier.MoveNext()) {
            resultName = valueAssembler.GenerateUniqueName(baseName, distinguifier.Current);
            if (uniqueNameEnsurer.IsUnique(resultName, out reservation)) return resultName;
        }
        // If all values are used, throw an exception
        throw new InvalidOperationException("Distinguifier values exceeded!");
    }

}

If you now like to create your UniqueFileNameGenerator, you would just inherit from UniqueNameGenerator and provide your logic. This could still be a framework class, but probably somewhere in a file system library and not in some common tools library as the ones above:

public class UniqueFileNameGenerator : UniqueNameGenerator<string, int, string, FileStream> {

    // Constructors

    public UniqueFileNameGenerator() 
        : this(null, null, null) {
    }

    public UniqueFileNameGenerator(IUniqueNameEnsurer<String, FileStream>? uniqueNameEnsurer, IEnumerable<Int32>? distinguifier, IUniqueNameAssembler<String, Int32, String>? uniqueNameAssembler) 
        : base(uniqueNameEnsurer ?? UniqueNameEnsurerDefaultImplementation, 
              distinguifier ?? DistinguifierDefaultImplementation, 
              uniqueNameAssembler ?? UniqueNameAssemblerDefaultImplementation) {
    }

    // Private Properties

    private static IUniqueNameEnsurer<string, FileStream> UniqueNameEnsurerDefaultImplementation = new UniqueNameEnsurer<string, FileStream>(fullPath => !File.Exists(fullPath), fullPath => {
        if (File.Exists(fullPath)) return null;
        try {
            string? parentFolder = Path.GetDirectoryName(fullPath);
            if (parentFolder is not null) {
                Directory.CreateDirectory(parentFolder!);
            }
            return File.OpenWrite(fullPath);
        } catch {
            // Ignore race conditions for the same name
            if (File.Exists(fullPath)) return null;
            // Rethrow exception
            throw;
        }
    });

    private static IEnumerable<Int32> DistinguifierDefaultImplementation { get; } = Enumerable.Range(1, Int32.MaxValue);

    private static IUniqueNameAssembler<string, int, string> UniqueNameAssemblerDefaultImplementation = new UniqueNameAssembler<string, int, string>(
        fullpath => fullpath, 
        (fullpath, number) => {
            string extension = Path.GetExtension(fullpath);
            string pathWithoutExtension = Path.Combine(Path.GetDirectoryName(fullpath)!, Path.GetFileNameWithoutExtension(fullpath));
            return $"{pathWithoutExtension} ({number.ToString(CultureInfo.InvariantCulture)}){extension}";
        }
    );

}

You then would use the class like this:

class Program {

    static void Main(string[] args) {
        string baseName = @"C:\Temp\NameGenerator\Foo.txt";
        Directory.CreateDirectory(Path.GetDirectoryName(baseName));

        UniqueFileNameGenerator uniqueFileNameGenerator2 = new UniqueFileNameGenerator();
        string uniqueName = uniqueFileNameGenerator2.GenerateUniqueName(baseName); //Unsafe
        string uniqueName2 = uniqueFileNameGenerator2.GenerateUniqueName(baseName, out FileStream fileStream); //Safe
        using (fileStream) {
            //Use the file stream to write to uniqueName2
            //....
        }
    }

}

With the same base classes you could now generate unique names in database, the file system, in the cloud by calling REST services etc.

\$\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.