just confuse about this

[marlyandedsel]

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

[Korvacs]

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.

[marlyandedsel]

yes it is part of the screen class.. I just want to know if this is good coding style or bad

[Korvacs]

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.

[shadowman123]

Originally Posted by Korvacs

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 ?

[pro4never]

Originally Posted by shadowman123

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.

[marlyandedsel]

how about if I do like this

PHP Code:

lock(_objects)
                            _objects.Add(_object.UID, _object);
                        UpdateArray();
                        return true; 


is it okay?

[pro4never]

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.