9
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Collections.Concurrent;

namespace Crystal_Message
{
    class Message
    {
        private int messageID;
        private string message;
        private ConcurrentBag <Employee> messageFor;
        private Person messageFrom;
        private string calltype;

        public Message(int iden,string message, Person messageFrom, string calltype, string telephone)
        {
            this.messageID = iden;
            this.messageFor = new ConcurrentBag<Employee>();
            this.Note = message;
            this.MessageFrom = messageFrom;
            this.CallType = calltype;
        }

        public ConcurrentBag<Employee> ReturnMessageFor
        {
            get
            {

                return messageFor;
            }

        }

        public int MessageIdentification
        {
            get { return this.messageID; }

            private set
            {
                if(value == 0)
                {
                    throw new ArgumentNullException("Must have Message ID");
                }

                this.messageID = value;
            }

        }

        public string Note
        {
            get { return message; }

            private set
            {
                if (string.IsNullOrWhiteSpace(value))
                {
                    throw new ArgumentException("Must Have a Message");

                }
                this.message = value;


            }

        }

        public Person MessageFrom
        {
            get { return messageFrom; }

            private set
            {
                this.messageFrom = value;
            }

        }

        public string CallType
        {
            get { return this.calltype; }

            private set
            {
                if (string.IsNullOrWhiteSpace(value))
                {
                    throw new ArgumentNullException("Please specify call type");
                }

                this.calltype = value;

            }

        }

        public void addEmployee(Employee add)
        {
            messageFor.Add(add);
        }


        public override string ToString()
        {
            return "Message: " + this.message + " From: " + this.messageFrom + " Call Type: " + this.calltype + " For: " + this.returnMessagefor();
        }

        private string returnMessagefor() 
        {
            string generate="";

            foreach(Employee view in messageFor)
            {
                generate += view.ToString() + " ";
            }

            return generate;
        }

        public override bool Equals(object obj)
        {
            if (obj == null)
            {
                return false;
            }

            Message testEquals = obj as Message;

            if((System.Object)testEquals == null)
            {
                return false;
            }

            return (this.messageID == testEquals.messageID) && (this.message == testEquals.message) && (this.messageFor == testEquals.messageFor) && (this.messageFrom == testEquals.messageFrom) && (this.calltype == testEquals.calltype);

        }

        public bool Equals(Message p)
        {
            if ((Object)p == null)
            {
                return false;
            }

            return (this.messageID == p.messageID) && (this.message == p.message) && (this.messageFor == p.messageFor) && (this.messageFrom == p.messageFrom) && (this.calltype == p.calltype);

        }

        public override int GetHashCode()
        {


            unchecked
            {
                return this.messageID.GetHashCode() * 33 ^ this.message.GetHashCode() * 33 ^ this.messageFor.GetHashCode() * 33 ^ this.messageFrom.GetHashCode() * 33 ^ this.calltype.GetHashCode();
            }


        }

    }
}

I have a Message class where a user could leave a message for more than one person. I have a getter for it, however, is returning a ConcurrentBag<> the way I've done proper practice? If not, how do i return the ConcurrentBag<> so I can loop through it and display it?

2 Answers2

7

ConcurrentBag<T> is an IEnumerable<T>. You can loop through it as usual. However, as this is a thread safe collection, there are performance concerns to using it.

If you want to get rid of the performance impact while looping, call ToArray on it and return the new array instead.

    public IEnumerable<Employee> ReturnMessageFor
    {
        get
        {

            return messageFor.ToArray();
        }

    }
Gusdor
  • 14,001
  • 2
  • 52
  • 64
  • Is the getter thread-safe though? What if performance isn't an issue? Could I leave the getter as it is? –  Jun 08 '15 at 12:08
  • @Nexusfactor leaving it as is is just fine but be aware that consuming code can then add new items to the bag. That is not always desirable. – Gusdor Jun 08 '15 at 12:15
  • The messageFor (as far as I can see in your code) will only be created once in the ctor. So no threading problems. Threading problems come into play when objects are mutable, but in case of collections the concurrent ones are explicitly taken care for these problems. Another question would be if you Employee class is mutable and what happens if two threads are working on them at the same time. – Oliver Jun 08 '15 at 12:15
  • That `ToArray()` is NOT a Linq extension method. It's built-in to `ConcurrentBag`. (If it was in fact a Linq extension, then it wouldn't get rid of the performance impact while looping, since Linq just does the equivalent of a foreach anwyay) – Matthew Watson Jun 08 '15 at 12:15
  • @MatthewWatson well spotted. However, an array is an array and is always looped sequentially and in place. The namespace of the method does not change that. Converting the array will incur ~the same overhead as one loop, but for many loops/uses, the array is cheaper. – Gusdor Jun 08 '15 at 12:19
  • Actually, the implementation in `ConcurrentBag<>` is more than just looping. It takes a snapshot of the bag, so it is locked while the copy is being made. It calls internal methods `FreezeBag()` and `UnfreezeBag()` to do so. – Matthew Watson Jun 08 '15 at 15:33
  • @MatthewWatson does that irrelevant implementation detail (it is still thread safe, right?) invalidate this answer? – Gusdor Jun 08 '15 at 15:41
  • It's very relevant - it means that it's an even better way of doing it! It means that the contents of the container can't change while it's being copied, which can be VERY important in some multithreading scenarios. Since this is a specificially concurrent container, anything relating to concurrency is VERY significant. – Matthew Watson Jun 08 '15 at 18:43
  • @MatthewWatson - Bare with me, as my threading knowledge isn't that strong. Alright, how would you accomplish the return of the collection in a tread-safe way? Can I leave it as is, if performance isn't an issue? Secondly, how do I make my employee thread safe, so that if two threads are indeed working on it, I don't run into an issue? Do I place lock around the getter/setters? –  Jun 08 '15 at 22:46
  • @Nexusfactor If you use this answer it will be threadsafe, because the implementation of the `ConcurrentBag.ToArray()` is threadsafe. So you can just use it as shown in this answer. – Matthew Watson Jun 09 '15 at 06:02
  • @MatthewWatson - Thanks. Just for my own curiousity, How would I make my employee object thread safe? You mentioned earlier that "Another question would be if you Employee class is mutable and what happens if two threads are working on them at the same time". How would I fix it? –  Jun 09 '15 at 13:13
  • @Nexusfactor Only access the object within a critical section. This is another question entirely. https://msdn.microsoft.com/en-us/library/c5kehkcz.aspx – Gusdor Jun 09 '15 at 14:00
  • @Gusdor - I would embed my getters/setter, and methods with the lock, correct? –  Jun 09 '15 at 14:10
  • 1
    @Nexusfactor please read the documentation carefully and ask another question if you are still stuck. – Gusdor Jun 09 '15 at 14:11
  • @Gusdor - Just for my own curiosity, I shall ask another question. I'll link it here, so you can have a look, and answer. –  Jun 09 '15 at 14:14
  • @Gusdor - Have a look: http://stackoverflow.com/questions/30734830/c-sharp-making-custom-object-thread-safe –  Jun 09 '15 at 14:22
2

It's not clear to me what you are trying to accomplish.

Are you trying to externalize the Bag for all operations? Because that's what you did...

If you want to externalize something you can iterate over you should either return the Bag as IEnumerable or return an array or a list copied from the Bag.

Either way it's safe to iterate over. Might not be the best in terms of performance, but that's another question.

    // Option 1
    public IEnumerable<Employee> ReturnMessageFor
    {
        get
        {
            return messageFor;
        }
    }

    // Option 2
    public Employee[] ReturnMessageFor
    {
        get
        {
            return messageFor.ToArray();
        }
    }

Notes:

  • You might want to make messageFor readonly (in the code you posted it is readonly).
  • Remember that a ConcurrentBag allows you to safely iterate over a snapshot of the collection in a thread safe manner, but it does not lock the items in the collection.
Yosef O
  • 367
  • 2
  • 7
  • All I wanted was to grab the collection and iterate over it. However, I don't want to the user to modify the collection elsewhere. I like your second approach. Returns an array, and you can't modify messagefor in anyway, correct? –  Jun 08 '15 at 22:47
  • Yep. ToArray return a _shallow copy_ of the collection so removeFor cannot be changed. – Yosef O Jun 09 '15 at 09:02