Preventing invalid entity state that contains two related collections

Question

I need to define an entity (CompositeContactList) that contains and manages two collections of identifiers (IncludedLists and ExcludedLists) that are ids of some other entities (ContactList).

The business rules are the following:
1. The collections must contain only unique identifiers.
2. The same identifier cannot belong to both IncludedLists and ExcludedLists.
3. Identifiers can be added or removed (performing the checks above).

Note: Adding an identifier to the ExcludedLists is not opposite to removing it from the IncludedLists. For instance, there is a list A that contains included lists B and C and excluded list D. That means that A = (B + C) - D. An attempt to add list C to the ExcludedLists leads to an error while C is in the IncludedLists. If the list C is removed from the IncludedLists, the list A will be the following: A = B - D.

I introduced two IEnumerable<Guid> properties to get the collections and four methods to manage them. There are few areas I'd like to hear some feedback about. Which variant is better?

Property naming

1. IncludedLists and ExcludedLists
2. IncludedListIds and ExcludedListIds

Method naming:

1. • IncludeList(id)
   • RemoveFromIncludes(id)
   • ExcludeList(id)
   • RemoveFromExcludes(id)
2. • AddIncludedList(id)
   • RemoveIncludedList(id)
   • AddExcludedList(id)
   • RemoveExcludedList(id)
3. • AddIncludedListId(id)
   • RemoveIncludedListId(id)
   • AddExcludedListId(id)
   • RemoveExcludedListId(id)

Validation

1. Rule 1. When I add the same identifier twice, does it may sense to throw an exception saying that the id cannot be added? Is there any harm in 'doing nothing' here?

2. Rule 2. Trying to add a list id to the included lists throws an error if it's already belongs to the excluded. Does it make sense to introduce some helping methods like CanAddToIncludedLists or TryAddToIncludedLists?

3. Does it make sense to throw an ArgumentException if an empty Guid is provided?

Usability

1. Does it make sense in declaring 'Add' methods as void and 'Remove' methods returning bool? (similar to .NET collections).

2. Does it make sense in adding other collection-specific methods like ClearIncludedLists or ClearAll?

Code

public class MyCompositeEntity
{
    private readonly ISet<Guid> _excludedLists = new HashSet<Guid>();
    private readonly ISet<Guid> _includedLists = new HashSet<Guid>();

    public MyCompositeEntity(Guid id)
    {
        if (id == Guid.Empty)
            throw new ArgumentException("The identifier cannot be empty.", nameof(id));

        Id = id;
    }

    public Guid Id { get; }

    public IEnumerable<Guid> IncludedLists => _includedLists;

    public IEnumerable<Guid> ExcludedLists => _excludedLists;

    public void IncludeList(Guid id)
    {
        if (id == Guid.Empty)
            throw new ArgumentException("The identifier cannot be empty.", nameof(id));

        if (ExcludedLists.Contains(id))
            throw new ArgumentException(
                "The identifier cannot be included because it is already excluded. " +
                "Please remove it from the excluded lists first.",
                nameof(id));

        _includedLists.Add(id);
    }

    public void ExcludeList(Guid id)
    {
        if (id == Guid.Empty)
            throw new ArgumentException("The identifier cannot be empty.", nameof(id));

        if (IncludedLists.Contains(id))
            throw new ArgumentException(
                "The identifier cannot be excluded because it is already included. " +
                "Please remove it from the included lists first.",
                nameof(id));

        _excludedLists.Add(id);
    }

    public bool RemoveFromIncludes(Guid id)
    {
        if (id == Guid.Empty)
            throw new ArgumentException("The identifier cannot be empty.", nameof(id));

        return _includedLists.Remove(id);
    }

    public bool RemoveFromExcludes(Guid id)
    {
        if (id == Guid.Empty)
            throw new ArgumentException("The identifier cannot be empty.", nameof(id));

        return _excludedLists.Remove(id);
    }
}

Answer 1

I would give these the same signature

public void IncludeList(Guid id)  

public bool RemoveFromIncludes(Guid id)

And rename

public bool AddToIncludes(Guid id)
{
    ...
    return _includedLists.Add(id);
}

Another approach is

Dictionary<GUID, bool>

and just one list

Answer 2

Validation

• Rule 1. When I add the same identifier twice, does it may sense to throw an exception saying that the id cannot be added? Is there any harm in 'doing nothing' here?

This question cannot be answered without knowing more about what you are trying to build. In some situations it migth be better to throw in others not to throw. If you are aiming for speed then not throwing is better and because you already use an ISet<T> and its Add method returns a bool then your API could do the same:

public bool IsExcluded(Guid id) => Excluded.Contains(id.ThrowIfEmpty());

public bool Include(Guid id)
{
    if (IsExcluded(id)) throw new InvalidOperationException("...");
    return _included.Add(id);
}

If someone tries to include and excluded id you should throw the InvalidOperationException which is more appropriate in this situation because the argument is valid but the operation is not so it's not a argument exception.

• Rule 2. Trying to add a list id to the included lists throws an error if it's already belongs to the excluded. Does it make sense to introduce some helping methods like CanAddToIncludedLists or TryAddToIncludedLists? Does it make sense to throw an ArgumentException if an empty Guid is provided? Usability

Yes, it does make sense because otherwise you have the same validation in several places. You could create a helper extension to validate the id so that you don't have to repeat yourself everywhere you need the validation.

static class ValidationExtensions
{
    public static Guid ThrowIfEmpty(this Guid id)
    {
        if (id == Guid.Empty) throw new ArgumentException("The identifier cannot be empty.", nameof(id));
        return id;
    }
}

For the other part of the question see answer 1. A helper method like IsExcluded makes sense and allows the user to check the id before including it and risking an error.

Usability

Does it make sense in declaring 'Add' methods as void and 'Remove' methods returning bool? (similar to .NET collections).

It depends on the collaction. Not all collection operations are void. Sets return a bool if the operation succeeded and you use sets so return what they return. It's nothing wrong in having some feedback expecially if you are planing to do something based on the result or performance is an important factor.

Does it make sense in adding other collection-specific methods like ClearIncludedLists or ClearAll?

Agian, it depends on what your are building. If you are never going to use them then why should you implement them? Its your custom collection to solve a particular problem and probably not a general purpose one so create only APIs that you will really use.