C#•8mo ago

✅ 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•8mo 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

LazyGuardOP•8mo ago

what do you mean by "making copy" here ? also, I am not sure I understand the "owns"

Jimmacle•8mo 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

LazyGuardOP•8mo ago

hum can you provide some code please that illustrates that bug ?

canton7•8mo 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

LazyGuardOP•8mo ago

why the value.ToHashSet(); is better ?

canton7•8mo ago

Shorter, clearer

LazyGuardOP•8mo ago

that's because we didn't use _reservations or _tags. right ?

canton7•8mo ago

What do you mean?

LazyGuardOP•8mo 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•8mo 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

LazyGuardOP•8mo ago

okay, one last question, do you try to avoid IEnumerable in general? What about IReadOnlyCollection rather than list ?

canton7•8mo 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

LazyGuardOP•8mo 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•8mo ago

that is the one i'm describing

LazyGuardOP•8mo 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•8mo 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...

LazyGuardOP•8mo 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•8mo 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?

LazyGuardOP•8mo 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•8mo ago

Correct

LazyGuardOP•8mo 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•8mo 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

LazyGuardOP•8mo 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•8mo 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

LazyGuardOP•8mo ago

yes I've got it, I was just contrasting with making the copy in the setter

canton7•8mo ago

That's worse than wrapping it in an immutable container

LazyGuardOP•8mo 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•8mo 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

LazyGuardOP•8mo ago

have or have not ?

canton7•8mo ago

*have now

LazyGuardOP•8mo ago

@canton7 do you have some refrences to such critiques ? blog articles, videos, whatever

canton7•8mo 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

LazyGuardOP•8mo ago

okay will dig for it, thanks a lot for all the answers it was very clear 🙏

Gaming

Programming

✅ How to modernize this code ?