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;
}
}
}
```
2 Answers 2
Potential infinite loop
If theisValid
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 whetheri
is less than that upper bound.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 usethis
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;
}
// ...
}
-
\$\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\$Enrique Moreno Tent– Enrique Moreno Tent2021年04月12日 09:50:49 +00:00Commented Apr 12, 2021 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. Eithernew NameGenerator("someName", MyIsValid)
orNameGenerator("someName", MyIsValid, 10)
without needing to declare multiple constructors \$\endgroup\$MindSwipe– MindSwipe2021年04月12日 10:19:16 +00:00Commented Apr 12, 2021 at 10:19
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.
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\$IsUnused
would express better its function. Thanks! \$\endgroup\$