1
\$\begingroup\$

I made up a quick and dirty implementation of the G-Set in C#. I was hoping for some feedback on my approach. I'm not totally happy with having the payload public but I can't see that I'm gaining much by making it private either.

public class GrowOnlySet<T>
{
    public HashSet<T> payload {get; private set;}

    public GrowOnlySet()
    {
        payload = new HashSet<T>();
    }

    public GrowOnlySet(HashSet<T> newstore)
    {
        payload = newstore;
    }

    public void Add(T element)
    {
        payload.Add(element);
    }

    public bool Lookup(T element)
    {
        return payload.Contains(element);
    }

    public bool Compare(GrowOnlySet<T> R)
    {
        return R.payload.IsSubsetOf(this.payload);
    }

    public void Merge(GrowOnlySet<T> R)
    {
        this.payload.UnionWith(R.payload);
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ How are you going to use it? \$\endgroup\$ – t3chb0t Jun 19 '18 at 16:06
  • \$\begingroup\$ I'm simply intending to implement that data structure (G-Set) as described on that Wikipedia entry. \$\endgroup\$ – Onorio Catenacci Jun 19 '18 at 17:17
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Malachi Jun 20 '18 at 13:08
  • \$\begingroup\$ if you would like further review of your new code please post a new question with the new updated code you have. \$\endgroup\$ – Malachi Jun 20 '18 at 13:09
4
\$\begingroup\$

I can't see that I'm gaining much by making it private either.

The clients of the GrowOnlySet might use this object in a inappropriate way. For example they can remove an element.

So I can do this, the compiler lets me do it.

var gset = new GrowOnlySet<string>();
gset.payload.Remove("element");

Now there is no grow only set anymore. The encapsulation is important.

Also as a client I really don't need to know the internal representation, which is the way how it was chose to represent the grow only set.

And because the payload property is public now the GrowOnlySet interface has a lot of other methods that I can use (via .payload... ), thus there is no abstraction. But I needed an interface made of only Lookup, Compare, Merge and Add methods.

There is not check for null newstore.

 public GrowOnlySet(HashSet<T> newstore)
    {
        payload = newstore;
    }

Thus the methods that are using the payload will throw a NullReferenceException which is not good. I prefer to throw a ArgumentNullException so I can create a consistent GrowOnlySet and to let the clients to be aware that they need to provide a new store.

 public bool Compare(GrowOnlySet<T> R)
    {
        return R.payload.IsSubsetOf(this.payload);
    }

Naming convention here. Suggested name is r. Or maybe find a better name like other (inspired from the HashSet itself)

this. is redundant, so it can be removed to make the code cleaner.

Compare and Merge accept null arguments but again this will throw NullReferenceException. Better is to inform the clients of this class that it really needs an object to be compared or to be merged with.

So the class is refactored to something like this

 public class GrowOnlySet<T>
    {
        private readonly HashSet<T> _payload;

        public GrowOnlySet()
        {
            _payload = new HashSet<T>();
        }

        public GrowOnlySet(HashSet<T> newstore)
        {            
            _payload = newstore ?? throw new ArgumentNullException(nameof(newstore));
        }

        public void Add(T element)
        {
            _payload.Add(element);
        }

        public bool Lookup(T element)
        {
            return _payload.Contains(element);
        }

        public bool Compare(GrowOnlySet<T> other)
        {
            if (other == null) throw new ArgumentNullException(nameof(other));

            return other._payload.IsSubsetOf(_payload);
        }

        public void Merge(GrowOnlySet<T> other)
        {
            if (other == null) throw new ArgumentNullException(nameof(other));

            _payload.UnionWith(other._payload);
        }
    }
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Excellent point regarding encapsulation. But am I missing something here? In your Compare you have other._payload.IsSubsetOf(_payload); but _payload is private. \$\endgroup\$ – Onorio Catenacci Jun 19 '18 at 19:11
  • 1
    \$\begingroup\$ @onorio in C# visibility is defined per class, not per object. It means that two objects (=instances) of the same class can access each other's private members. \$\endgroup\$ – Adriano Repetti Jun 19 '18 at 21:59
  • \$\begingroup\$ Wow--I never realized that. I guess I've never tried to do that so I just never found it out. That seems to be yet another violation of encapsulation. \$\endgroup\$ – Onorio Catenacci Jun 20 '18 at 13:05
  • \$\begingroup\$ You can see it as an encapsulation at the class level and not at the object level. \$\endgroup\$ – Adrian Iftode Jun 20 '18 at 13:43

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.