current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

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.

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 
{
}

Questions:

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

Thanks

share|improve this question
    
Looks fine to me –  Nikita Brizhak Oct 1 '13 at 6:58

1 Answer 1

up vote 1 down vote accepted

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:

share|improve this answer
    
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? –  georgepiva Oct 3 '13 at 12:19
    
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. –  0xBADF00D Oct 7 '13 at 7:50
    
Ok, thank @hichaeretaqua for your answer. –  georgepiva Oct 7 '13 at 12:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.