Refinded reviewer assigment

* Fixed bug when computing assignments based on existing assignments
* Use efficient and simple algorithm when no predefined assigments exist.

src/TutorBot.Cli/Domain/Assignment.cs

@@ -113,19 +113,26 @@ public static async Task<Assignment> FromGitHub(IGitHubClassroomClient client, I
 
   public IReadOnlyList<(Submission, Student)> FindReviewers()
   {
-    var validSubmissions = Submissions.Where(s => s.Assessment.IsValid()).ToList();
-    validSubmissions.Shuffle();
+    try
+    {
+      var validSubmissions = Submissions.Where(s => s.Assessment.IsValid()).ToList();
+      validSubmissions.Shuffle();
+
+      var studentToSubmission = new Dictionary<Student, Submission>();
+      validSubmissions.ForEach(s => studentToSubmission.Add(s.Owner, s));
 
-    var studentToSubmission = new Dictionary<Student, Submission>();
-    validSubmissions.ForEach(s => studentToSubmission.Add(s.Owner, s));
+      var existingAssignments = validSubmissions.Where(s => s.Reviewers.Count > 0).Select(s => (s, studentToSubmission[s.Reviewers[0]]));
 
-    var existingAssignments = validSubmissions.Where(s => s.Reviewers.Count > 0).Select(s => (s, studentToSubmission[s.Reviewers[0]]));
-
-    var mapping = new EntityMapper<Submission>(validSubmissions, existingAssignments);
+      var mapping = new EntityMapper<Submission>(validSubmissions, existingAssignments);
 
-    return mapping.FindUniqueMapping()
-                  .Select(pair => (pair.Key, pair.Value.Owner))
-                  .ToList().AsReadOnly();
+      return mapping.FindUniqueMapping()
+                    .Select(pair => (pair.Key, pair.Value.Owner))
+                    .ToList().AsReadOnly();
+    } 
+    catch (NonUniqueValuesException<Submission> ex)
+    {
+      throw new ReviewerAssignmentException($"Reviewers are assigned to multiple submissions: {ex.NonUniqueValues.ToStringWithSeparator()}");
+    }
   }
 
   public async Task AssignReviewers(IEnumerable<(Submission, Student)> reviewers, IProgress? progress = null)

src/TutorBot.Cli/Domain/EntityMapper.cs

@@ -1,90 +1,147 @@
-using System;
-using System.Collections.Generic;
-using System.Formats.Asn1;
-using System.Linq;
-using System.Reflection;
-using System.Text;
-using System.Threading.Tasks;
+using TutorBot.Domain.Exceptions;
 
 namespace TutorBot.Domain;
 
 public class EntityMapper<T> where T : notnull
 {
   private readonly IList<T> entities;
-  private readonly IDictionary<T, T> givenMappings;
-  private readonly IDictionary<T, int> orderId;
+  private readonly IDictionary<T, T> givenMapping;
 
-  private IEnumerable<T> RotatedList(List<T> list, int first)
+
+  public EntityMapper(IEnumerable<T> entities, IEnumerable<(T, T)> givenMappings)
   {
-    if (first < 0 || first >= list.Count)
+    this.entities = entities.ToList();
+
+    this.givenMapping = new Dictionary<T, T>();
+    var m = givenMappings.ToList();
+    foreach (var (source, target) in givenMappings)
     {
-      throw new IndexOutOfRangeException($"frist out of range [0, {list.Count}");
+      this.givenMapping.Add(source, target);
     }
 
-    for (int i = 0, index = first; i < list.Count; i++, index = (index + 1) % list.Count)
+    CheckIfMappingIsValid(givenMapping);
+  }
+
+  public IDictionary<T, T> FindUniqueMapping()
+  {
+    if (givenMapping.Count == 0)
     {
-      yield return list[index];
+      return FindUniqueMappingWithoutPredefinitions();
+    }
+    else
+    {
+      return FindUniqueMappingWithPredefinitions();
     }
   }
 
-  private int FindNextIndexModular(List<T> entities, T start)
+  private T FindEndOfPath(IDictionary<T, T> mapping, T start)
   {
-    int first = entities.FindIndex(t => orderId[t] > orderId[start]);
-    if (first == -1)
+    var current = start;
+    while (mapping.TryGetValue(current, out var next))
     {
-      first = entities.FindIndex(t => orderId[t] < orderId[start]);
+      current = next;
     }
 
-    return first;
+    return current;
   }
 
-  public EntityMapper(IEnumerable<T> entities, IEnumerable<(T, T)> givenMappings)
+  private bool IsCycle(IDictionary<T, T> mapping, T source, T target)
   {
-    this.entities = entities.ToList();
-
-    this.givenMappings = new Dictionary<T, T>();
-    var m = givenMappings.ToList();
-    foreach (var (source, target) in givenMappings)
+    var unionMapping = new Dictionary<T, T>();
+    foreach (var (s, t) in givenMapping)
+    {
+      unionMapping.Add(s, t);
+    }
+    foreach (var (s, t) in mapping)
     {
-      this.givenMappings.Add(source, target);
+      unionMapping.Add(s, t);
     }
+    unionMapping.Add(source, target);
 
-    this.orderId = new Dictionary<T, int>(this.entities.Count);
-    for (int i = 0; i < this.entities.Count; i++)
+    var current = source;
+    while (unionMapping.TryGetValue(current, out var next))
     {
-      orderId.Add(this.entities[i], i);
+      if (next.Equals(source))
+      {
+        return true;
+      }
+      current = next;
     }
+    return false;
   }
 
-
-  public IDictionary<T, T> FindUniqueMapping()
+  private void CheckIfMappingIsValid(IDictionary<T, T> mapping)
   {
-    var mapping = new Dictionary<T, T>();
+    var inDegree = new Dictionary<T, int>();
+    foreach (var (_, target) in mapping)
+    {
+      inDegree[target] = inDegree.GetValueOrDefault(target) + 1;
+    }
 
-    var unmappedEntities = entities.Where(e => !givenMappings.ContainsKey(e)).ToList();
+    var nonUniqueTargets = new Dictionary<T, int>();
+    var notUniqueTargets = inDegree.Where(kvp => kvp.Value > 1).Select(kvp => kvp.Key).ToList();
+    if (notUniqueTargets.Count > 0)
+    {
+      throw new NonUniqueValuesException<T>("EntityMapper was passed an invalid mapping.",
+                                                   notUniqueTargets);
+    }
+  }
 
-    var untargetedEntitySet = new HashSet<T>(entities);
-    untargetedEntitySet.RemoveWhere(givenMappings.Values.Contains);
-    var untargetedEntities = entities.Where(e => untargetedEntitySet.Contains(e)).ToList();
+  private IDictionary<T, T> FindUniqueMappingWithoutPredefinitions()
+  {
+    var mapping = new Dictionary<T, T>();
+    if (entities.Count <= 1)
+    {
+      return mapping;
+    }
 
-    if (unmappedEntities.Count != untargetedEntities.Count)
+    for (var i = 0; i < entities.Count; i++)
     {
-      throw new InvalidOperationException("Unexpected internal state: Number of sources must be equal to number of targets");
+      var j = (i + 1) % entities.Count;
+      mapping.Add(entities[i], entities[j]);
     }
 
-    if (unmappedEntities.Count == 0 ||
-        (unmappedEntities.Count == 1 && unmappedEntities[0].Equals(untargetedEntities[0])))
+    return mapping;
+  }
+
+  private IDictionary<T, T> FindUniqueMappingWithPredefinitions()
+  {
+    var mapping = new Dictionary<T, T>();
+
+    var unmappedEntities = entities.Where(e => !givenMapping.ContainsKey(e)).ToList();
+    if (unmappedEntities.Count == 0)
     {
       return mapping;
     }
 
-    int first = FindNextIndexModular(untargetedEntities, unmappedEntities[0]);
-    var targets = RotatedList(untargetedEntities, first).GetEnumerator();
+    var untargetedEntities = new HashSet<T>(entities);
+    untargetedEntities.RemoveWhere(givenMapping.Values.Contains);
+
+    var start = untargetedEntities.First();
+    untargetedEntities.Remove(start);
+    var current = FindEndOfPath(givenMapping, start);
+
+    while (untargetedEntities.Count > 0)
+    {
+      var successor = untargetedEntities.First();
+
+      foreach (var target in untargetedEntities)
+      {
+        if (!IsCycle(mapping, current, target))
+        {
+          successor = target;
+          break;
+        }
+      }
+
+      untargetedEntities.Remove(successor);
+      mapping.Add(current, successor);
+      current = FindEndOfPath(givenMapping, successor);
+    }
 
-    foreach (var source in unmappedEntities)
+    if (!current.Equals(start))
     {
-      targets.MoveNext();
-      mapping.Add(source, targets.Current);
+      mapping.Add(current, start); // close the cycle
     }
 
     return mapping;

src/TutorBot.Cli/Domain/Exceptions/AssessmentFileException.cs

@@ -1,6 +1,4 @@
-using TutorBot.Infrastructure.Exceptions;
-
-namespace TutorBot.Domain.Exceptions;
+namespace TutorBot.Domain.Exceptions;
 
 public class AssessmentFileException : DomainException
 {

src/TutorBot.Cli/Domain/Exceptions/NonUniqueValuesException.cs

@@ -0,0 +1,13 @@
+using TutorBot.Infrastructure.Exceptions;
+
+namespace TutorBot.Domain.Exceptions;
+
+public class NonUniqueValuesException<T> : DomainException
+{
+  public IEnumerable<T> NonUniqueValues { get; }
+
+  public NonUniqueValuesException(string message, IEnumerable<T> nonUniqueValues) : base(message)
+  {
+    NonUniqueValues = nonUniqueValues;
+  }
+}

src/TutorBot.Cli/Domain/Exceptions/ReviewerAssignmentException.cs

@@ -0,0 +1,8 @@
+namespace TutorBot.Domain.Exceptions;
+
+public class ReviewerAssignmentException : DomainException
+{
+  public ReviewerAssignmentException(string message) : base(message)
+  {
+  }
+}

src/TutorBot.Cli/TutorBot.Cli.csproj

@@ -7,7 +7,7 @@
     <ImplicitUsings>enable</ImplicitUsings>
     <Nullable>enable</Nullable>
     <AssemblyName>gh-tutorbot</AssemblyName>
-    <Version>0.6.0</Version>
+    <Version>0.6.1</Version>
   </PropertyGroup>
 
   <ItemGroup>

src/TutorBot.Tests/AssignReviewersTests.cs

@@ -137,7 +137,7 @@ public async Task ReviewerAssignment_WithTwoSubmission_ShouldBeCorrect()
 
     IList<(Submission Submission, Student Reviewer)> reviewers = assignment.FindReviewers().ToList();
     reviewers.Should().HaveCount(2);
-    reviewers[0].Submission.Owner.Should().Be(reviewers[1].Reviewer);
+    reviewers[0].Submission.Owner.Should().BeEquivalentTo(reviewers[1].Reviewer);
   }
 
   [Theory]
@@ -194,13 +194,13 @@ public async Task ReviewerAssignment_WithThreeSubmissionsAndOnePreAssignedReview
   [InlineData(5)]
   [InlineData(10)]
   [InlineData(30)]
-  public async Task ReviewerAssignment_WithPreAssignedReviewers_ShouldBeCorrect(int numSubmission)
+  public async Task ReviewerAssignment_WithPreAssignedReviewers_ShouldBeCorrect(int numSubmissions)
   {
-    var students = CreateStudentList(numSubmission);
+    var students = CreateStudentList(numSubmissions);
     var emptyReviewerList = new List<Reviewer>();
 
     var submissions = new List<Submission>();
-    for (int i = 0; i < numSubmission; i++)
+    for (int i = 0; i < numSubmissions; i++)
     {
       if (i % 2 == 0)
       {

src/TutorBot.Tests/EntityMapperTests.cs

@@ -3,6 +3,20 @@
 
 namespace TutorBot.Tests;
 
+public static class CollectionExtensions
+{
+  public static void DeterministicShuffle<T>(this IList<T> list, int seed)
+  {
+    var random = new Random(seed);
+
+    for (int i = list.Count - 1; i > 0; i--)
+    {
+      int k = random.Next(i);
+      (list[k], list[i]) = (list[i], list[k]);
+    }
+  }
+}
+
 public class EntityMapperTests
 {
   private Func<string, string, bool> suffixEqual = (s, t) => !s[1..].Equals(t[1..]);
@@ -11,7 +25,7 @@ private static IEnumerable<string> CreateStrings(string prefix, int num)
   {
     for (int i = 0; i < num; i++)
     {
-      yield return $"S{prefix}{i}";
+      yield return $"{prefix}{i}";
     }
   }
 
@@ -63,8 +77,8 @@ public void Mapping_WithTwoEntities_ShouldBeCorrect()
     var mapping = mapper.FindUniqueMapping();
 
     mapping.Should().HaveCount(2);
-    mapping["E1"].Should().Be("E2");
-    mapping["E2"].Should().Be("E1");
+    mapping["E1"].Should().BeEquivalentTo("E2");
+    mapping["E2"].Should().BeEquivalentTo("E1");
   }
 
   [Fact]
@@ -77,8 +91,8 @@ public void Mapping_WithTwoEntitiesInReverseOrder_ShouldBeCorrect()
     var mapping = mapper.FindUniqueMapping();
 
     mapping.Should().HaveCount(2);
-    mapping["E1"].Should().Be("E2");
-    mapping["E2"].Should().Be("E1");
+    mapping["E1"].Should().BeEquivalentTo("E2");
+    mapping["E2"].Should().BeEquivalentTo("E1");
   }
 
 
@@ -136,6 +150,35 @@ public void Mapping_WithTreeEntitiesAndTwoGivenMappings_ShouldBeCorrect()
     MappingShouldBeCorrect(entities.ToList(), resultingMapping);
   }
 
+  [Fact]
+  public void Mapping_StressTest()
+  {
+    for (int nEntities = 5; nEntities <= 8; nEntities++)
+    {
+      for (var i = 0; i < 10; i++)
+      {
+        var entities = CreateStrings("E", nEntities).ToList();
+        var givenMapping = new List<(string, string)>
+        {
+          ("E1", "E0"),
+          ("E0", "E2"),
+          ("E3", "E4")
+        };
+        entities.DeterministicShuffle(i * 10);
+
+        var mapper = new EntityMapper<string>(entities, givenMapping);
+
+        var newMapping = mapper.FindUniqueMapping();
+
+        var resultingMapping = new Dictionary<string, string>();
+        foreach (var (s, t) in givenMapping) resultingMapping.Add(s, t);
+        foreach (var (s, t) in newMapping) resultingMapping.Add(s, t);
+
+        MappingShouldBeCorrect(entities.ToList(), resultingMapping);
+      }
+    }
+  }
+
   [Fact]
   public void Mapping_WithGivenCycle_ShouldBeCorrect()
   {