Message Shows Up Multiple Times Inside Inbox

Question

I have a PM system that is more like the Google Mail style. I mean messages are grouped into conversations. If a user receives a message, it shows up in their inbox. Also, the this user sends a message to someone who in turn replies back, then this message also shows up in their inbox.

Some messages are being retrieved in both queries which end up being grouped into one List. I tried to remove duplicates by doing what Jon Skeet did in removing duplicates from a list C# but still, I keep getting duplicates. Here's my code:

UPDATED:

public class Message : IEquatable<Message>
{
    public int Id { get; set; }
    [MaxLength(150)]
    public string Subject { get; set; }
    [MaxLength(3000)]
    public string Content { get; set; }
    public DateTime DateSent { get; set; }
    public DateTime? LastViewed { get; set; }
    public bool IsRead { get; set; }
    public bool DisplayInInbox { get; set; }
    public virtual User SentBy { get; set; }
    public virtual User ReceivedBy { get; set; }
    public int? ParentId { get; set; }

    public override bool Equals(object other)
    {
        return Equals(other as Message);
    }

    public bool Equals(Message other)
    {
        if (object.ReferenceEquals(other, null))
        {
            return false;
        }
        if (object.ReferenceEquals(other, this))
        {
            return true;
        }
        return Id == other.Id;
    }

    public override int GetHashCode()
    {
        return this.Id;
    }
}

//inside MessagingService public IList GetThreads(User user) { //Get all messages that are not replies. var tmp = _repository.GetMany(c => c.DisplayInInbox.Equals(true) && c.ParentId.Equals(null)); var threads = (from c in tmp where GetReplies(user, c.Id).Count() > 0 select c).ToList(); var threadsByUser = user.ReceivedMessages.Where(m => m.DisplayInInbox.Equals(true) && m.ParentId.Equals(null)).ToList(); threads.AddRange(threadsByUser); threads.Distinct().ToList(); return threads; }

Am I doing something wrong here?

Answer 1

In both DTO's you have implemented GatHashcode. Shouldn't you be using these when testing equality?

return Id == other.Id && Subject == other.Subject && SentBy.Nickname == other.SentBy.Nickname &&
           DateSent == other.DateSent;

return Id == other.Id && Subject == other.Subject && Sender == other.Sender;

become

return GetHashCode() == other.GetHashCode()

EDIT:

I'm also slightly being a biff... don't override/overload the equals method. It is the equals method that uses GetHashcode to determine equality. You have overloaded equals to catch the DTO, which by default would have compared the result of GetHashcode on both objects. Your overloaded version does not compare the hashcode, making it redundant, when actually it looks like a correct implementation.

EDIT 2 (in response to code changes in your post): It's hard to tell because the code section at the bottom of your post is not formatted, but the second to last line:

threads.Distinct().ToList();
return threads;

This doesn't do anything. Merge the two:

return threads.Distinct().ToList();

Answer 2

Without providing us any example of your duplicate messages that are not being removed, I am going to guess that your usage of 'DateTime' in your equality checking code is a likely culprit. It is very common, and easy to write something like 'SentDate = DateTime.Now;' and have that confuse the system later on down the line. That is just a guess though.

Meanwhile I would suggest that your equality and hashcode functions are way way into the territory of overkill. Now I am going to assume that the 'ID' for both the Message and the MessageThread classes are supposed to be unique, in which case you can really simpify your logic, and more easily get to the bottom of your problem, like so;

public override bool Equals(object other)
{
    return Equals(other as Message);
}

public bool Equals(Message other)
{
  if (other == null) { return false; }
  return this.Id == other.Id;
}

public override int GetHashCode()
{
  // If the ID is unique, then it satisfied the purpose of 'GetHashCode'
  return this.Id;
}

Obviously you will want to do this for your other classes as well. The benefit here is that there are less moving parts, and consequently less opportunity for something to go awry. Also, the message bodies, senders, etc. are not needed for you to determine the uniqueness of the message.

Answer 3

Is this ok?

return Id == other.Id && Subject == other.Subject && SentBy.Nickname == other.SentBy.Nickname &&
               DateSent == other.DateSent;

I would prefer approach that if id equals, mail messages is same:

return Id == other.Id;