Change the internal backing list to a SortedSet to reduce complexity & increase stability

RhinoReminds/ClientListener.cs

@@ -9,7 +9,6 @@ using System.Threading;
 using System.Threading.Tasks;
 using RhinoReminds.Utilities;
 using S22.Xmpp;
-using S22.Xmpp.Client;
 using S22.Xmpp.Im;
 using SBRL.Geometry;
 
@@ -34,20 +33,20 @@ namespace RhinoReminds
 		public string ReminderFilePath { get; set; } = "./reminders.xml";
 		private ReminderList reminderList = new ReminderList();
 		private CancellationTokenSource reminderWatcherReset;
-		private CancellationToken reminderWatcherResetToken;
+		private CancellationToken reminderWatcherResetToken => reminderWatcherReset.Token;
 
 		private SimpleXmppClient client;
 		/// <summary>
 		/// The initial number of seconds to wait before trying to reconnect to the 
 		/// server again if we loose our connection again.
 		/// </summary>
-		private int defaultBackoffDelay = 1;
+		private readonly int defaultBackoffDelay = 1;
-		private float backoffDelayMultiplier = 2;
+		private readonly float backoffDelayMultiplier = 2;
 		/// <summary>
 		/// If a connection attempt doesn't succeed in this number of seconds,
 		/// give up and try again later.
 		/// </summary>
-		private int giveUpTimeout = 30;
+		private readonly int giveUpTimeout = 30;
 
 
 		public ClientListener(string inJid, string inPassword)
@@ -226,7 +225,7 @@ namespace RhinoReminds
 					}
 					if (succeeded.Count > 0) {
 						// Ensure that the reminder thread picks up the changes
-						resetReminderWatcher();
+						interruptReminderWatcher();
 						string response = string.Join(", ", succeeded.Select((int nextId) => $"#{nextId}"));
 						response = $"Deleted reminder{(succeeded.Count != 1 ? "s" : "")} {response} successfully.";
 						client.SendChatReply(message, response);
@@ -236,7 +235,7 @@ namespace RhinoReminds
 				case "list":
 				case "show":
 					// Filter by reminders for this user.
-					IEnumerable<Reminder> userReminderList = reminderList.Reminders.Values.Where(
+					IEnumerable<Reminder> userReminderList = reminderList.Reminders.Where(
 						(Reminder next) => message.From.GetBareJid() == next.JidObj.GetBareJid()
 					);
 					StringBuilder listMessage = new StringBuilder("I've got the following reminders on my list:\n");
@@ -283,9 +282,19 @@ namespace RhinoReminds
 						@"and"
 					}, RegexOptions.IgnoreCase).Trim();
 
-					client.SendChatReply(message, $"Ok! I'll remind you {reminder} at {dateTime}.");
 
 					Reminder newReminder = reminderList.CreateReminder(message.From, dateTime, reminder);
+					if (newReminder == null) {
+						client.SendChatReply(
+							message,
+							"Oops! It looks like you've already got a reminder idential "
+							+ "to that one set, so I wasn't able to set a reminder for you. Please contact my "
+							+ "operator, as this is probably a bug."
+						);
+						break;
+					}
+
+					client.SendChatReply(message, $"Ok! I'll remind you {reminder} at {dateTime}.");
 					reminderList.Save(ReminderFilePath);
 					break;
 
@@ -302,17 +311,19 @@ namespace RhinoReminds
 
 		#region Reminder Listening
 
-		private void resetReminderWatcher()
+		private void interruptReminderWatcher()
 		{
-			// We don't need to create a new token here, as the watcher does thisi automagically
+			reminderWatcherReset?.Cancel();
-			reminderWatcherReset.Cancel();
+			reminderWatcherReset = new CancellationTokenSource();
 		}
 
 		private async Task watchForReminders()
 		{
-			reminderWatcherReset = new CancellationTokenSource();
+			interruptReminderWatcher();
-			reminderWatcherResetToken = reminderWatcherReset.Token;
 			Reminder nextReminder = reminderList.GetNextReminder();
+
+			// ----- Events -----
+			// This will run on the firing thread, not on this thread
 			reminderList.OnReminderListUpdate += (object sender, Reminder newReminder) => {
 				Reminder newNextReminder = reminderList.GetNextReminder();
 				//Console.WriteLine("[Rhino/Reminderd/Canceller] Reminder added - comparing.");
@@ -320,52 +331,46 @@ namespace RhinoReminds
 				if (nextReminder != newNextReminder) {
 					//Console.WriteLine($"[Rhino/Reminderd/Canceller] Cancelling");
 					nextReminder = newNextReminder;
-					reminderWatcherReset.Cancel();
+					interruptReminderWatcher();
 				}
 			};
+			// ------------------
 
-			while (true)
+			while (true) {
-			{
+				nextReminder = reminderList.GetNextReminder();
+				// Wait for the next reminder
 				TimeSpan nextWaitingTime;
 				try {
 					if (nextReminder != null) {
 						nextWaitingTime = nextReminder.Time - DateTime.Now;
-						if (DateTime.Now < nextReminder.Time) {
+						if (DateTime.Now < nextReminder.Time)
+						{
 							//Console.WriteLine($"[Rhino/Reminderd] Sleeping for {nextWaitingTime}");
 							await Task.Delay(nextWaitingTime, reminderWatcherResetToken);
 						}
-					} else {
+					}
+					else {
 						//Console.WriteLine("[Rhino/Reminderd] Sleeping until interrupted");
 						await Task.Delay(Timeout.Infinite, reminderWatcherResetToken);
 					}
-				} catch (TaskCanceledException) {
+				}
+				catch (TaskCanceledException)
+				{
 					//Console.WriteLine("[Rhino/Reminderd] Sleep interrupted, recalculating");
-					reminderWatcherReset = new CancellationTokenSource();
+					interruptReminderWatcher();
-					reminderWatcherResetToken = reminderWatcherReset.Token;
 					continue;
 				}
 
-				if (reminderWatcherResetToken.IsCancellationRequested) {
+				if (reminderWatcherResetToken.IsCancellationRequested)
+				{
 					Console.WriteLine("[Rhino/Reminderd] Sleep interrupted, recalculating (but no exception thrown)");
-					reminderWatcherReset = new CancellationTokenSource();
+					interruptReminderWatcher();
-					reminderWatcherResetToken = reminderWatcherReset.Token;
-
 					continue;
 				}
 
 				Console.WriteLine($"[Rhino/Reminderd] Sending notification {nextReminder}");
+				sendAndDeleteReminder(nextReminder);
 
-				try
-				{
-					client.SendChatMessage(
-						nextReminder.Jid,
-						$"Hello! You asked me to remind you {nextReminder.Message} at {nextReminder.Time}.".Trim().Replace(@"\s+", " ")
-					);
-				} catch (Exception error) {
-					Console.Error.WriteLine($"[Rhino/Reminderd] Caught error sending message to client: {error}");
-					Console.Error.WriteLine($"[Rhink/Reminderd] Offending reminder: {nextReminder}");
-					client.SendChatMessage(nextReminder.Jid, "Oops! I encountered an error sending you a reminder. Please contact my operator!");
-				}
 				if (nextWaitingTime.TotalMilliseconds < 0) {
 					client.SendChatMessage(
 						nextReminder.Jid,
@@ -373,10 +378,27 @@ namespace RhinoReminds
 						"or you might have scheduled a reminder for the past)"
 					);
 				}
+			}
+		}
+
+		private void sendAndDeleteReminder(Reminder nextReminder)
+		{
+			try
+			{
+				client.SendChatMessage(
+					nextReminder.Jid,
+					$"Hello! You asked me to remind you {nextReminder.Message} at {nextReminder.Time}.".Trim().Replace(@"\s+", " ")
+				);
+			}
+			catch (Exception error)
+			{
+				Console.Error.WriteLine($"[Rhino/Reminderd] Caught error sending message to client: {error}");
+				Console.Error.WriteLine($"[Rhink/Reminderd] Offending reminder: {nextReminder}");
+				client.SendChatMessage(nextReminder.Jid, "Oops! I encountered an error sending you a reminder. Please contact my operator!");
+			}
+
 			reminderList.DeleteReminder(nextReminder);
 			reminderList.Save(ReminderFilePath);
-				nextReminder = reminderList.GetNextReminder();
-			}
 		}
 
 		#endregion

RhinoReminds/CompareReminders.cs

@@ -0,0 +1,17 @@
+using System;
+using System.Collections.Generic;
+
+namespace RhinoReminds
+{
+	public class CompareReminders : IComparer<Reminder>
+	{
+		public CompareReminders()
+		{
+		}
+
+		public int Compare(Reminder x, Reminder y)
+		{
+			return (int)(x.Time - y.Time).TotalMilliseconds;
+		}
+	}
+}

RhinoReminds/Reminder.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using System.Net;
 using System.Xml;
 using S22.Xmpp;
@@ -6,29 +7,59 @@ using S22.Xmpp;
 namespace RhinoReminds
 {
 
-	public class Reminder
+	public class Reminder : IEquatable<Reminder>
 	{
 		public int Id { get; }
-		public string Jid { get; }
+		public string Jid => JidObj.ToString();
-		public Jid JidObj => new Jid(Jid);
+		public Jid JidObj { get; }
-		public DateTime Time { get; }
+		public DateTime Time { get; private set; }
 		public string Message { get; }
 
-		public Reminder(int inId, string inJid, DateTime inTime, string inMessage)
+		public Reminder(int inId, Jid inJid, DateTime inTime, string inMessage)
 		{
 			Id = inId;
-			Jid = inJid;
+			JidObj = inJid.GetBareJid();
 			Time = inTime;
 			Message = inMessage;
 		}
 
-		public override string ToString()
+		public void TweakTime() {
-		{
+			Time = Time.AddMilliseconds(1);
-			return $"[Reminder Id={Id}, Jid={Jid}, Time={Time}, Message={Message}";
 		}
 
-		#region XML
 
+		#region Overrides
+
+		public override bool Equals(object obj) {
+			if (!(obj is Reminder)) return false;
+			Reminder otherReminder = obj as Reminder;
+			return otherReminder.Id == Id &&
+				otherReminder.JidObj == JidObj && // Will *always* be a bare Jid
+				otherReminder.Time == Time &&
+				otherReminder.Message == Message;
+		}
+		// For IEquatable<Reminder> implementation
+		public bool Equals(Reminder otherReminder) => Equals((object)otherReminder);
+
+		public override int GetHashCode() {
+			int hashCode = -81903051;
+			hashCode = (hashCode * -1521134295) + Id.GetHashCode();
+			hashCode = (hashCode * -1521134295) + EqualityComparer<Jid>.Default.GetHashCode(JidObj);
+			hashCode = (hashCode * -1521134295) + Time.GetHashCode();
+			hashCode = (hashCode * -1521134295) + EqualityComparer<string>.Default.GetHashCode(Message);
+			return hashCode;
+		}
+		public static bool operator ==(Reminder reminder1, Reminder reminder2) => EqualityComparer<Reminder>.Default.Equals(reminder1, reminder2);
+		public static bool operator !=(Reminder reminder1, Reminder reminder2) => !(reminder1 == reminder2);
+
+		public override string ToString() {
+			return $"[Reminder Id={Id}, Jid={Jid}, Time={Time}, Message=\"{Message}\"]";
+		}
+
+		#endregion
+
+
+		#region XML
 
 		public void WriteToXml(XmlWriter xml)
 		{
@@ -66,7 +97,8 @@ namespace RhinoReminds
 			return new Reminder(id, jid, dateTime, message);
 		}
 
-
 		#endregion
+
+
 	}
 }

RhinoReminds/ReminderList.cs

@@ -1,6 +1,5 @@
 using System;
 using System.Collections.Generic;
-using System.IO;
 using System.Linq;
 using System.Xml;
 using S22.Xmpp;
@@ -14,7 +13,7 @@ namespace RhinoReminds
 		private int nextId = 0;
 		private readonly object saveLock = new object();
 
-		public SortedList<DateTime, Reminder> Reminders = new SortedList<DateTime, Reminder>();
+		public SortedSet<Reminder> Reminders = new SortedSet<Reminder>(new CompareReminders());
 
 		public event OnReminderListUpdateHandler OnReminderListUpdate;
 
@@ -29,29 +28,28 @@ namespace RhinoReminds
 		{
 			Reminder result = new Reminder(nextId++, $"{inJid.Node}@{inJid.Domain}", time, message);
 			Console.WriteLine($"[Rhino/ReminderList] Created reminder {result}");
-			while (Reminders.ContainsKey(time))
+			while (Reminders.Contains(result))
-				time = time.AddMilliseconds(1);
+				result.TweakTime();
-			Reminders.Add(time, result);
+
+			Reminders.Add(result);
 			OnReminderListUpdate(this, result);
 			return result;
 		}
 
-		public Reminder GetNextReminder()
+		public Reminder GetNextReminder() {
-		{
 			if (Reminders.Count == 0)
 				return null;
 
-			return Reminders.Values[0];
+			return Reminders.Min;
 		}
 
-		public Reminder GetById(int targetId)
+		public Reminder GetById(int targetId) {
-		{
+			return Reminders.First((Reminder nextReminder) => nextReminder.Id == targetId);
-			return Reminders.First((KeyValuePair<DateTime, Reminder> nextPair) => nextPair.Value.Id == targetId).Value;
 		}
 
-		public void DeleteReminder(Reminder nextReminder)
+		public void DeleteReminder(Reminder nextReminder) {
-		{
+			if (!Reminders.Remove(nextReminder))
-			Reminders.Remove(nextReminder.Time);
+				throw new ApplicationException($"Error: Failed to remove the reminder {nextReminder} from the list!");
 		}
 
 
@@ -62,7 +60,7 @@ namespace RhinoReminds
 			// Make sure that the reminder thread doesn't try to save the reminders at the exact same time
 			// we receive a request for a new reminder
 			lock (saveLock)
-			{ // FUTURE: We could go lockless here with some work, but it's not worth it for the teeny chance & low overhead 
+			{ // FUTURE: We could go lockless here with some work, but it's not worth it for the teensy chance & low overhead 
 				XmlWriter xml = XmlWriter.Create(
 					filename,
 					new XmlWriterSettings() { Indent = true }
@@ -74,7 +72,7 @@ namespace RhinoReminds
 				xml.WriteElementString("NextId", nextId.ToString());
 
 				xml.WriteStartElement("Reminders");
-				foreach (Reminder nextReminder in Reminders.Values)
+				foreach (Reminder nextReminder in Reminders)
 					nextReminder.WriteToXml(xml);
 				xml.WriteEndElement();
 
@@ -89,15 +87,15 @@ namespace RhinoReminds
 			XmlDocument xml = new XmlDocument();
 			xml.Load(filepath);
 
-			ReminderList result = new ReminderList();
+			ReminderList result = new ReminderList() {
-			result.nextId = int.Parse(xml.GetElementsByTagName("NextId")[0].InnerText);
+				nextId = int.Parse(xml.GetElementsByTagName("NextId")[0].InnerText)
-			foreach (XmlNode reminderXML in xml.GetElementsByTagName("Reminders")[0].ChildNodes)
+			};
-			{
+
+			foreach (XmlNode reminderXML in xml.GetElementsByTagName("Reminders")[0].ChildNodes) {
 				Reminder nextReminder = Reminder.FromXml(reminderXML);
-				DateTime timeKey = nextReminder.Time;
+				while (result.Reminders.Contains(nextReminder))
-				while (result.Reminders.ContainsKey(timeKey))
+					nextReminder.TweakTime();
-					timeKey = timeKey.AddMilliseconds(1);
+				result.Reminders.Add(nextReminder);
-				result.Reminders.Add(timeKey, nextReminder);
 			}
 
 			return result;

RhinoReminds/RhinoReminds.csproj

@@ -69,6 +69,7 @@
     <Compile Include="Utilities\Range.cs" />
     <Compile Include="Utilities\TextHelpers.cs" />
     <Compile Include="SimpleXmppClient.cs" />
+    <Compile Include="CompareReminders.cs" />
   </ItemGroup>
   <ItemGroup>
     <None Include="packages.config" />