just confuse about this

09/10/2012 04:27 marlyandedsel#1
can you guys tell me whats good for this code and tell me if this is good or not...Im really learning here but sometime just got confuse or something....

PHP Code:
 public bool Add(Interfaces.IMapObject _object)
        {
            try
            {                
                if (!_objects.ContainsKey(_object.UID))
                {
                    if (ServerBase.Kernel.GetDistance(_object.X, _object.Y, Owner.Entity.X, Owner.Entity.Y) <= ServerBase.Constants.pScreenDistance)
                    {
                        _objects.Add(_object.UID, _object);
                        UpdateArray();
                        return true;
                    }
                }
                else
                {
                    if (_objects[_object.UID] == null)
                    {
                        _objects.Remove(_object.UID);
                        if (ServerBase.Kernel.GetDistance(_object.X, _object.Y, Owner.Entity.X, Owner.Entity.Y) <= ServerBase.Constants.pScreenDistance)
                        {
                            _objects.Add(_object.UID, _object);
                            UpdateArray();
                            return true;
                        }
                    }
                }                
            }
            catch (Exception e) { Program.SaveException(e); }
          //  finally { /*Monitor.Exit(_objects);*/ }
            return false;
        }

        public bool Remove(Interfaces.IMapObject _object)
        {
            try
            {
                //if (Monitor.TryEnter(_objects, 10))
                //{
                if (_objects.ContainsKey(_object.UID))
                {
                    if (_object.MapObjType == MapObjectType.Item)
                    {
                        Network.GamePackets.FloorItem item = _object as Network.GamePackets.FloorItem;
                        item.Type = Network.GamePackets.FloorItem.Remove;
                        item.SendSpawn(Owner, false);
                        item.Type = Network.GamePackets.FloorItem.Drop;
                    }
                    else if (_object.MapObjType == MapObjectType.Player)
                    {
                        Owner.Send(new Network.GamePackets.Data(true) { UID = _object.UID, ID = Network.GamePackets.Data.RemoveEntity });
                    }
                    _objects.Remove(_object.UID);
                    UpdateArray();
                    return true;
                }
                //}
            }
            catch (Exception e) { Program.SaveException(e); }
           // finally { /*Monitor.Exit(_objects);*/ }
            return false;
        } 
Thank you in advance
09/10/2012 11:41 Korvacs#2
That code forms part of a wrapper for a dictionary that adds and removes objects and updates the clients at the same time. I suspect this is part of a screen class.
09/10/2012 15:31 marlyandedsel#3
yes it is part of the screen class.. I just want to know if this is good coding style or bad
09/10/2012 16:06 Korvacs#4
Pros and cons to it, its not very concurrent so i would suggest more cons than pros. Its not what i would describe as bad code however.
09/10/2012 18:54 shadowman123#5
Quote:
Originally Posted by Korvacs View Post
Pros and cons to it, its not very concurrent so i would suggest more cons than pros. Its not what i would describe as bad code however.
could u tell me what are cons / Pros ?
09/10/2012 22:54 pro4never#6
Quote:
Originally Posted by shadowman123 View Post
could u tell me what are cons / Pros ?
He already said it's not concurrent. Therefor it is going to have issues when you start having multiple threads accessing non thread safe resources.

You're not locking collections or using any concurrent ones.

Simplest solution IMO would be to convert to a ConcurrentCollection so that things cannot be screwed up while you iterate through them.
09/12/2012 04:21 marlyandedsel#7
how about if I do like this
PHP Code:
lock(_objects)
                            _objects.Add(_object.UID, _object);
                        UpdateArray();
                        return true; 
is it okay?
09/12/2012 14:16 pro4never#8
Locking creates its own set of issues (deadlocks as well as inefficiency of threads waiting for their turn for objects to be released)

As I said... just use ConcurrentCollections such as ConcurrentDictionary or whatever and you'll be much better off.