C
C#9mo ago
LazyGuard

✅ How to modernize this code ?

So, I have found this .Net Core 3.1 code and want to make it more modern.
using System.Collections;
using System.Runtime.CompilerServices;
using Chocco.Services.Availability.Core.Exceptions;
using Chocco.Services.Availability.Core.ValueObjects;

namespace Chocco.Services.Availability.Core.Entities;

public class Resource : AggregateRoot
{
private ISet<string> _tags = new HashSet<string>();
private ISet<Reservation> _reservations = new HashSet<Reservation>();

public IEnumerable<string> Tags
{
get => _tags;
private set => _tags = new HashSet<string>(value);
}

public IEnumerable<Reservation> Reservations
{
get => _reservations;
private set => _reservations = new HashSet<Reservation>(value);
}

public Resource(AggregateId id, IEnumerable<string> tags, IEnumerable<Reservation>? reservations = null, int version = 0)
{
var enumerable = tags.ToList();

ValidateTags(enumerable);
Id = id;
Tags = enumerable;
Reservations = reservations ?? Enumerable.Empty<Reservation>();
Version = version;
}

private static void ValidateTags(IEnumerable<string> tags)
{
var enumerable = tags.ToList();
if (tags is null || !enumerable.Any())
{
throw new MissingResourceTagsException();
}

if (enumerable.Any(string.IsNullOrWhiteSpace))
{
throw new InvalidResourceTagsException();
}
}
}
using System.Collections;
using System.Runtime.CompilerServices;
using Chocco.Services.Availability.Core.Exceptions;
using Chocco.Services.Availability.Core.ValueObjects;

namespace Chocco.Services.Availability.Core.Entities;

public class Resource : AggregateRoot
{
private ISet<string> _tags = new HashSet<string>();
private ISet<Reservation> _reservations = new HashSet<Reservation>();

public IEnumerable<string> Tags
{
get => _tags;
private set => _tags = new HashSet<string>(value);
}

public IEnumerable<Reservation> Reservations
{
get => _reservations;
private set => _reservations = new HashSet<Reservation>(value);
}

public Resource(AggregateId id, IEnumerable<string> tags, IEnumerable<Reservation>? reservations = null, int version = 0)
{
var enumerable = tags.ToList();

ValidateTags(enumerable);
Id = id;
Tags = enumerable;
Reservations = reservations ?? Enumerable.Empty<Reservation>();
Version = version;
}

private static void ValidateTags(IEnumerable<string> tags)
{
var enumerable = tags.ToList();
if (tags is null || !enumerable.Any())
{
throw new MissingResourceTagsException();
}

if (enumerable.Any(string.IsNullOrWhiteSpace))
{
throw new InvalidResourceTagsException();
}
}
}
PArticulary, what I don't like is that converting tags into List. IF I remove it the compiler warns me about multiple possible enumeration (what the heck is this !) Another thing is there a way to keep exposing an IEnumerable but having Sets as implementation? Any other suggestions are also welcome as well
34 Replies
Jimmacle
Jimmacle9mo ago
all an enumerable is is "a sequence of items" and could be doing all sorts of transformations/logic behind the scenes in order to provide that sequence so it warns you if you might enumerate it more than once because that could be expensive depending on what the enumerable actually is, or not work at all in this case it looks like the fact it makes a copy is also intentional so that the Resource actually "owns" the collection of tags it's using for its property
LazyGuard
LazyGuardOP9mo ago
what do you mean by "making copy" here ? also, I am not sure I understand the "owns"
Jimmacle
Jimmacle9mo ago
you don't know where the tags enumerable is coming from or if it will be modified again from outside this class ToList makes a copy (as a List<string>) that can't be changed from outside code that stops potential bugs like creating this resource with valid tags then adding an invalid tag to the list later
LazyGuard
LazyGuardOP9mo ago
hum can you provide some code please that illustrates that bug ?
canton7
canton79mo ago
Note, there's no point _tags or _reservations being interface types. You don't gain anything by hiding the HashSet behind an interface for private code, and you lose some speed. Just do private HashSet<string> _tags = new() Also, in the Tags setter, private set => _tags = value.ToHashSet() If ValidateTags is only called from the ctor, then yes using tags.ToList() there is wasteful, as the ctor has already done .ToList() Also, if that's your entire class, just make your properties getter-only. No need to make them mutable:
public class Resource : AggregateRoot
{
public IEnumerable<string> Tags { get; }
public IEnumerable<Reservation> Reservations { get; }

public Resource(AggregateId id, IEnumerable<string> tags, IEnumerable<Reservation>? reservations = null, int version = 0)
{
var tagsSet = tags.ToHashSet();
ValidateTags(tagsSet);

Id = id;
Tags = tagsSet;
Reservations = reservations?.ToHashSet() ?? new Enumerable.Empty<string>();
Version = version;
}

private static void ValidateTags(HashSet<string> tags)
{
if (tags is null || !enumerable.Any())
{
throw new MissingResourceTagsException();
}

if (enumerable.Any(string.IsNullOrWhiteSpace))
{
throw new InvalidResourceTagsException();
}
}
}
public class Resource : AggregateRoot
{
public IEnumerable<string> Tags { get; }
public IEnumerable<Reservation> Reservations { get; }

public Resource(AggregateId id, IEnumerable<string> tags, IEnumerable<Reservation>? reservations = null, int version = 0)
{
var tagsSet = tags.ToHashSet();
ValidateTags(tagsSet);

Id = id;
Tags = tagsSet;
Reservations = reservations?.ToHashSet() ?? new Enumerable.Empty<string>();
Version = version;
}

private static void ValidateTags(HashSet<string> tags)
{
if (tags is null || !enumerable.Any())
{
throw new MissingResourceTagsException();
}

if (enumerable.Any(string.IsNullOrWhiteSpace))
{
throw new InvalidResourceTagsException();
}
}
}
That said actually, I'd be tempted to expose your tags / reservations as either an IReadOnlyList if you want list-like behaviour (such as being able to access elements by index), or IReadOnlySet if you want set-like behaviour (such as testing set membership). IEnumerable just awkward to work with
LazyGuard
LazyGuardOP9mo ago
why the value.ToHashSet(); is better ?
canton7
canton79mo ago
Shorter, clearer
LazyGuard
LazyGuardOP9mo ago
that's because we didn't use _reservations or _tags. right ?
canton7
canton79mo ago
What do you mean?
LazyGuard
LazyGuardOP9mo ago
Did you mean that we can make the properties in the class as read-only (getter-only) because private setters are not used within the class? right>
canton7
canton79mo ago
Yes They're only used from the ctor. So it would be clearer if you do the version to set / etc directly in the ctor, rather than hiding it in the setter
LazyGuard
LazyGuardOP9mo ago
okay, one last question, do you try to avoid IEnumerable in general? What about IReadOnlyCollection rather than list ?
canton7
canton79mo ago
In general, if you can give a more useful interface to consumers, it's a bit nicer to. If you just expose an IEnumerable for example, people can't tell how many tags there are. If you just expose an IReadOnlyCollection, people can't fetch the 3rd tag, etc Since the list of tags is fixed and known, it's a bit nicer if you can give that information to consumers, rather than hiding it for no reason
LazyGuard
LazyGuardOP9mo ago
okay, that's very clear .. thanks but isn't the constructor already making a copy. I mean can't see how it's possible to change it from the external world
Jimmacle
Jimmacle9mo ago
that is the one i'm describing
LazyGuard
LazyGuardOP9mo ago
@jIMMACLE
there is no need to making a copy because with the following code
public class Resource : AggregateRoot
{
private HashSet<string> _tags = new();
private HashSet<Reservation> _reservations = new();

public IEnumerable<string> Tags
{
get => _tags;
set => _tags = new HashSet<string>(value);
}

public IEnumerable<Reservation> Reservations
{
get => _reservations;
private set => _reservations = new HashSet<Reservation>(value);
}

public Resource(AggregateId id, IEnumerable<string> tags, IEnumerable<Reservation>? reservations = null, int version = 0)
{
// var enumerableTags = tags.ToHashSet();
ValidateTags(tags);
Id = id;
Tags = tags;
Reservations = reservations ?? Enumerable.Empty<Reservation>();
Version = version;
}

private static void ValidateTags(IEnumerable<string> tags)
{
if (tags != null && (tags is null || !tags.Any()))
{
throw new MissingResourceTagsException();
}

if (tags.Any(string.IsNullOrWhiteSpace))
{
throw new InvalidResourceTagsException();
}
}
}
public class Resource : AggregateRoot
{
private HashSet<string> _tags = new();
private HashSet<Reservation> _reservations = new();

public IEnumerable<string> Tags
{
get => _tags;
set => _tags = new HashSet<string>(value);
}

public IEnumerable<Reservation> Reservations
{
get => _reservations;
private set => _reservations = new HashSet<Reservation>(value);
}

public Resource(AggregateId id, IEnumerable<string> tags, IEnumerable<Reservation>? reservations = null, int version = 0)
{
// var enumerableTags = tags.ToHashSet();
ValidateTags(tags);
Id = id;
Tags = tags;
Reservations = reservations ?? Enumerable.Empty<Reservation>();
Version = version;
}

private static void ValidateTags(IEnumerable<string> tags)
{
if (tags != null && (tags is null || !tags.Any()))
{
throw new MissingResourceTagsException();
}

if (tags.Any(string.IsNullOrWhiteSpace))
{
throw new InvalidResourceTagsException();
}
}
}
If the caller does
var liste = new List<string> { "validTag1", "validTag2" };
Console.WriteLine(liste);
var resource = new Resource(new AggregateId(), liste);
var tags = resource.Tags as HashSet<string>;
liste.Add("invalidTag3"); // Adding an invalid tag
Console.WriteLine(string.Join(",", resource.Tags)); //validTag1,validTag2
var liste = new List<string> { "validTag1", "validTag2" };
Console.WriteLine(liste);
var resource = new Resource(new AggregateId(), liste);
var tags = resource.Tags as HashSet<string>;
liste.Add("invalidTag3"); // Adding an invalid tag
Console.WriteLine(string.Join(",", resource.Tags)); //validTag1,validTag2
then the result is validTag1,validTag2
canton7
canton79mo ago
You've removed both ToLists, not just one of them The question, what's the danger in removing one, since there's a second one Yes if you remove both, you've got a problem...
LazyGuard
LazyGuardOP9mo ago
can't see where is the problem. I removed both ToList so my code does not do any copy! However with a code like this
var liste = new List<string> { "validTag1", "validTag2" };
Console.WriteLine(liste);
var resource = new Resource(new AggregateId(), liste);
var tags = resource.Tags as HashSet<string>;
liste.Add("invalidTag3"); // Adding an invalid tag
Console.WriteLine(string.Join(",", resource.Tags)); //validTag1,validTag2
var liste = new List<string> { "validTag1", "validTag2" };
Console.WriteLine(liste);
var resource = new Resource(new AggregateId(), liste);
var tags = resource.Tags as HashSet<string>;
liste.Add("invalidTag3"); // Adding an invalid tag
Console.WriteLine(string.Join(",", resource.Tags)); //validTag1,validTag2
Even though I touched the liste variable after creating the resouce, it didn't touch the resource Alos even when doing a ToList
var tagsList = tags.ToList();
ValidateTags(tagsList);
Id = id;
Tags = tagsList;
Reservations = reservations ?? Enumerable.Empty<Reservation>();
Version = version;
var tagsList = tags.ToList();
ValidateTags(tagsList);
Id = id;
Tags = tagsList;
Reservations = reservations ?? Enumerable.Empty<Reservation>();
Version = version;
A code like this would result in a bug
var resource = new Resource(new AggregateId(), new List<string> { "validTag1", "validTag2" });
var tags = resource.Tags as HashSet<string>;
tags.Add("invalidTag3");
Console.WriteLine(string.Join(",", resource.Tags));
var resource = new Resource(new AggregateId(), new List<string> { "validTag1", "validTag2" });
var tags = resource.Tags as HashSet<string>;
tags.Add("invalidTag3");
Console.WriteLine(string.Join(",", resource.Tags));
canton7
canton79mo ago
Yeah, but most code assumes that people don't go around randomly casting things Heck, if you want to bypass restrictions, there's always reflection. You can't avoid people doing that, so why try and avoid people casting types they don't own?
LazyGuard
LazyGuardOP9mo ago
I think there is different subjects here. I sill simplify the code as well to be more clear.
public class Resource : AggregateRoot
{
private HashSet<string> _tags;
public IEnumerable<string> Tags
{
get => _tags;
private set => _tags = value.ToHashSet();
}

public Resource(AggregateId id, IEnumerable<string> tags)
{
ValidateTags(tags);
Tags = tags;
}

private static void ValidateTags(IEnumerable<string> tags)
{
if (tags is null || !tags.Any())
{
throw new MissingResourceTagsException();
}

if (tags.Any(string.IsNullOrWhiteSpace))
{
throw new InvalidResourceTagsException();
}
}
}
public class Resource : AggregateRoot
{
private HashSet<string> _tags;
public IEnumerable<string> Tags
{
get => _tags;
private set => _tags = value.ToHashSet();
}

public Resource(AggregateId id, IEnumerable<string> tags)
{
ValidateTags(tags);
Tags = tags;
}

private static void ValidateTags(IEnumerable<string> tags)
{
if (tags is null || !tags.Any())
{
throw new MissingResourceTagsException();
}

if (tags.Any(string.IsNullOrWhiteSpace))
{
throw new InvalidResourceTagsException();
}
}
}
First case:
var inputList = new List<string> { "validTag1", "validTag2" };
var resource = new Resource(new AggregateId(), inputList);
inputList.Add("invalidTagInInput");
Console.WriteLine(string.Join(",", resource.Tags));
var inputList = new List<string> { "validTag1", "validTag2" };
var resource = new Resource(new AggregateId(), inputList);
inputList.Add("invalidTagInInput");
Console.WriteLine(string.Join(",", resource.Tags));
Here even though we touch the inputList after creating the Resource, it doesn't affect the resource's tags, that's because the result is validTag1,validTag2. That's because the ToHashSet in the setter creates a copy of the inputList right ?
canton7
canton79mo ago
Correct
LazyGuard
LazyGuardOP9mo ago
so my remark to @jIMMACLE is taht it's not the ToList that making the Resource class owning the tags because even when removing them, the ToHashSet in the setter is still protecting as. Now with the second case, now there is a completely different issue and that is, with the same code if the caller do this:
var resource = new Resource(new AggregateId(), new List<string> { "validTag1", "validTag2" });
var tags = resource.Tags as HashSet<string>;
tags.Add("invalidTag3");
Console.WriteLine(string.Join(",", resource.Tags));
var resource = new Resource(new AggregateId(), new List<string> { "validTag1", "validTag2" });
var tags = resource.Tags as HashSet<string>;
tags.Add("invalidTag3");
Console.WriteLine(string.Join(",", resource.Tags));
This will mutate the resource's tags. It will result in validTag1,validTag2,invalidTag3. So how to prevent this ? is it worth preventing ? Since we are making a copy in the setter, why we don't a mke a copy in the getter as well, thus we make the Resouce truly owns the tags
canton7
canton79mo ago
Yeah, but most code assumes that people don't go around randomly casting things Heck, if you want to bypass restrictions, there's always reflection. You can't avoid people doing that, so why try and avoid people casting types they don't own? Use .ToReadOnly() or an ImmuableArray to raise the barrier to malicious code a bit, but you can't prevent someone using reflection to change the set of tags
LazyGuard
LazyGuardOP9mo ago
do you implicitly mean that doing the copy in the setter is more important than in the getter ? since the bug is "more possible" it doesn't involve casting etc, right ?
canton7
canton79mo ago
I'm saying you should not copy in the getter. Nobody does that. It's pointlessly complex, and breaks the rule that fetching a property twice should result in two things which compare equal, if nothing else has changed If you want to expose something that's immutable, it's fine to hide it behind an immutable interface. If you really want to dissuade people from mutating it, wrap the underlying type in a read-only wrapper But, even that doesn't stop someone from actively changing it through reflection. So if your aim is to stop actively malicious code, you're fighting a losing battle
LazyGuard
LazyGuardOP9mo ago
yes I've got it, I was just contrasting with making the copy in the setter
canton7
canton79mo ago
That's worse than wrapping it in an immutable container
LazyGuard
LazyGuardOP9mo ago
I see ! I didn't know about the rule that fetching two things should result the same if nothing was changed here it's clearly referential equality right ? since value equality is guaranteed if nothing has changed
canton7
canton79mo ago
Yeah, you could look at that two ways. But the general idea is that a property should more-or-less behave as if it's just data, even if you're re-calculating it on demand. So it shouldn't just change on its own (DateTime.Now etc have not been widely regarded as bad design, for that reason) Now, you can argue semantics on reference equality vs value equality there.... But the sense of the guideline is that things shouldn't be needlessly changing, and new-ing up a new set each time a property is accessed is IMO a needless change
LazyGuard
LazyGuardOP9mo ago
have or have not ?
canton7
canton79mo ago
*have now
LazyGuard
LazyGuardOP9mo ago
@canton7 do you have some refrences to such critiques ? blog articles, videos, whatever
canton7
canton79mo ago
Nothing off the top of my head. There was probably something on Eric Lippert's blog at some point, but I'll have read it years and years ago
LazyGuard
LazyGuardOP9mo ago
okay will dig for it, thanks a lot for all the answers it was very clear 🙏
Want results from more Discord servers?
Add your server