3
\$\begingroup\$

By having two model classes Conversation and Message, what are the best practices to handle the next situation: A conversation listening for its messages PropertyChanged events and so being able to update itself.

  1. What are the best practices?
  2. How can I improve this model design?
  3. Is it going to generate memory leaks?

code

using SoftConsept.Collections;

public class Conversation 
{
    readonly SortedObservableCollection<Message> messages;

    public Conversation ()
    {
        messages = new SortedObservableCollection<Message> ();
    }

    public void Add (Message message)
    {
        messages.Add (message);
        message.PropertyChanged += HandleMessagePropertyChanged;
    }

    public void Remove (Message message)
    {
        message.PropertyChanged -= HandleMessagePropertyChanged;
        messages.Remove (message);
    }

    public IList<Message> Messages ()
    {
        return messages.ToList ();
    }

    void HandleMessagePropertyChanged (object sender, PropertyChangedEventArgs e)
    {
        // Uptade omitted conversations properties using data from the updated message.
    }
}

public class Message : INotifyPropertyChanged 
{
}
\$\endgroup\$
1
  • \$\begingroup\$ Looks fine to me \$\endgroup\$
    – Nikita B
    Oct 1, 2013 at 6:58

2 Answers 2

2
\$\begingroup\$

First of all, the Conversation class does not provide any events for chances, so nobody has a chance to get to know if something changes. This is already be implemented within an (Sorted)Observable collection, but unfortunately you publish this private field as IList<Message> by the Message method. So the notification abilities of this collection can not be used. Furthermore methods should always be connected with a action, so GetMessages() would be a more suitable name. But the best way is to make it a property.

using SoftConsept.Collections;

public class Conversation : INotifyPropertyChanged
{
    readonly SortedObservableCollection<Message> messages;

    public event PropertyChangedEventHandler PropertyChanged;

    public Conversation ()
    {
        messages = new SortedObservableCollection<Message> ();
    }

    private DateTime _updateTime = DateTime.Now;
    public DateTime UpdateTime{
       get{ return _updateTime;}
       private set{
           UpdateTime = value;
           OnPropertyChanged("UpdateTime");
       }
    }

    private void OnPropertyChanged(string propertyName){
       var handler = PropertyChanged;
       if(handler==null) return;
       handler(this, new PropertyChangedEventArgs(propertyName));
    }

    public void Add (Message message)
    {
        messages.Add (message);
        UpdateTime = DateTime.Now;
        message.PropertyChanged += OnMessagePropertyChanged;
    }

    private void OnMessagePropertyChanged(object sender, PropertyChangedEventArgs args)
    {
        UpdateTime = DateTime.Now;
    }

    public void Remove (Message message)
    {
        messages.Remove (message);
        UpdateTime = DateTime.Now;
        message.PropertyChanged -= OnMessagePropertyChanged;
    }

    public ObservableCollection<Message> Messages 
    {
        get{ 
            //I assume that SortedObservableCollection is subtype of ObservableCollection
            return messages;
        }
    }
}

public class Message : INotifyPropertyChanged {}

So now some observer might register for changes in Messages (add, delete) and also for changes in the single Message instances.

For more info, look at Microsoft documentation:

\$\endgroup\$
3
  • \$\begingroup\$ I really appreciated your answer. But just another question. Supposing that the Conversation class implements INotifyPropertyChanged as well and have the UpdatedDate property. How would you update its UpdatedDate value whenever a new message was added/updated inside the Messages collection? Would your Conversation listen for Messages changes or would you create another controller object to sync a Conversation properties with its current Messages collection state? \$\endgroup\$
    – georgepiva
    Oct 3, 2013 at 12:19
  • \$\begingroup\$ I recently changed the implementation to fulfill your requirements. Its basically the same, you already started with, but with the addition of setting the UpdateTime property manually, to invoke PropertyChangedEvent while adding and removing a message and also when something within the messages changes. \$\endgroup\$
    – 0xBADF00D
    Oct 7, 2013 at 7:50
  • \$\begingroup\$ Ok, thank @hichaeretaqua for your answer. \$\endgroup\$
    – georgepiva
    Oct 7, 2013 at 12:02
2
\$\begingroup\$

There are two possible memory leaks likely to occur.

  • You should implement the IDisposable interface or provide an alternative way of deattaching an object from events subscribed to referenced dependencies and/or event listeners.
  • You should let the container objects dealing with Converstation and Message instances handle the lifetime of these objects correctly.

Conversation could keep Message instances alive and vice versa.

public void Clear() 
{
    foreach (var message in messages) 
    {
        message.PropertyChanged -= OnMessagePropertyChanged;
    }
    messages.Clear();
}

public void Dispose()
{
    // .. dispose pattern impl
    Clear();
}

Message could keep any listener alive and vice versa.

public void Reset() 
{
    PropertyChanged = null;
}

public void Dispose()
{
    // .. dispose pattern impl
    Reset();
}

Consider using Weak Event Pattern if you don't want a strong reference between an object and its event listeners.

\$\endgroup\$

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.