Simple way to prevent duplicate logging of errors

I've been tasked with updating our current error logging system to prevent the logging of similar errors (from a single client program) that were probably caused by the same event. The code here is what I have added in order to accomplish this. Are there any obvious issues I am missing or is there a simpler way to accomplish this? I need to deal with multiple threads.

Here is the simple error object we use:

public class ErrorObject
{
    //to use when sending an object to a background worker
    public ErrorObject(Exception exception, string additionalinfo,DateTime? dateTime = null,bool showTouser = false)
    {
        Exception = exception;
        AdditionalInfo = additionalinfo;
        Time = dateTime ?? DateTime.Now;
    }

    public Exception Exception { get; set; }
    public string AdditionalInfo { get; set; }
    public DateTime Time { get; set; }
    public bool ShownToUser { get; set; }
}

Other code calls this function to log errors.

```
public static Int32 LogError(Exception ex, bool runInBackground = true, string additionalInfo = "",
bool ShownToUser = false)
{

int retVal = -1;
if (runInBackground)
{
ErrorObject newError = new ErrorObject(ex,additionalInfo,DateTime.Now,ShownToUser);

ThreadPool.QueueUserWorkItem(LogErrorInBackground, newError);
}
else
{
try
{ //Do a Sql Insert in current thread (slow for remote labs...)
retVal = InsertError(ex, additionalInfo, ShownToUser);
}
catch{}
}
return retVal;
}

//Store 4 previous errors to compare against
private static Queue PreviousErrors = new Queue(4);
private static object PreviousErrorLock = new object();

private static void LogErrorInBackground(object state)
{
ErrorObject error = (ErrorObject) state;

if (! QueueNewError(error))
{
InsertError(error.Exception, error.AdditionalInfo, error.ShownToUser);
}
}

/// Adds an error to the PreviousErrors Queue and removes the er

ErrorObject newError = new ErrorObject(…);

This is one of the cases where most people will probably agree that you can use var here. Doing that doesn't lose any information (you can still see at a glance that newError is an ErrorObject) and it makes the code shorter and more DRY.

return retVal;

In general, you shouldn't use integer retVals. If you need to report errors, use exceptions. If you want to report success or failure, use a bool. If you want to report multiple possible results, use an enum. If you want to report some kind of count (though I don't see how would that make sense here), at least name your variable something like count, not retVal.

//Store 4 previous errors to compare against
private static Queue PreviousErrors = new Queue(4);

This sounds like a job for a simple reusable custom collection. Something like:

class BoundedQueue : IEnumerable
{
    private readonly int count;
    private readonly Queue queue;

    public BoundedQueue(int count)
    {
        this.count = count;
        this.queue = new Queue(count);
    }

    public void Enqueue(T item)
    {
        if (queue.Count == count)
            queue.Dequeue();

        queue.Enqueue(item);
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public IEnumerator GetEnumerator()
    {
        return queue.GetEnumerator();
    }
}

lock (PreviousErrorLock)

Since you want to lock access to the PreviousErrors object, you should use that in the lock, you don't need another object for this. (An object used just for locking makes sense when you logically want to do something like lock (this), but that's not the case here.)

foreach (ErrorObject previousError in PreviousErrors)
{
    if (previousError.AdditionalInfo == newError.AdditionalInfo &&
    Math.Abs((previousError.Time - newError.Time).TotalSeconds) < 3 &&
    previousError.Exception.GetType() == newError.Exception.GetType() &&
    previousError.Exception.Message == newError.Exception.Message &&
    previousError.Exception.TargetSite == newError.Exception.TargetSite)
    {
        isDuplicate = true;
        break;//Exit the loop
    }
}

You should extract this equality testing code into a separate method, possibly as ErrorObject.Equals(). With that, this code would simplify to:

bool isDuplicate = PreviousErrors.Contains(newError);

break;//Exit the loop

You don't need this comment. Everyone knows what break does inside a loop.

Also, I find the queue logic suspicious: when you detect a duplicate, you add it to the queue anyway. So if the sequence of exceptions is something like 1, 2, 2, 2, 2, 1, then you're going to report 1 twice, even though it's among the 2 most recent error kinds.

StackExchange Code Review Q#57307, answer score: 2