From 0772ebbcd8e4853bc677c335216ccbc892aedb6e Mon Sep 17 00:00:00 2001 From: Ibrahim Alhas Date: Thu, 12 Sep 2024 16:14:17 +0100 Subject: [PATCH 1/6] fixed error with timeLimit = 0 with blackouts used leading to division by zero Additionally, refactored code to make it more robust and readable --- Assets/Scripts/Lights.cs | 124 ++++++++++++++++++++++++++------------- 1 file changed, 84 insertions(+), 40 deletions(-) diff --git a/Assets/Scripts/Lights.cs b/Assets/Scripts/Lights.cs index f8c4930..41d924e 100644 --- a/Assets/Scripts/Lights.cs +++ b/Assets/Scripts/Lights.cs @@ -3,16 +3,18 @@ using System.Collections.Generic; /// -/// Facilitates the management and retrieval of Fade components associated with a collection of GameObjects, black screens. +/// Script for handling the lights in the environment (blackouts). /// namespace Lights { public class InfiniteEnumerator : IEnumerator { - private int _initialValue; - private int _currentValue; + private readonly int _initialValue = 0; + private int _currentValue = 0; - public InfiniteEnumerator(int initialValue = 0) + public InfiniteEnumerator() { } + + public InfiniteEnumerator(int initialValue) { _initialValue = initialValue; _currentValue = 0; @@ -29,70 +31,112 @@ public void Reset() _currentValue = 0; } - public int Current => _currentValue; + public int Current + { + get { return _currentValue; } + } - object IEnumerator.Current => Current; + object IEnumerator.Current + { + get { return Current; } + } - public void Dispose() { } + void IDisposable.Dispose() { } } - public class LightsSwitch { - private int _episodeLength; + private readonly int _episodeLength = 0; private bool _lightStatus = true; - private List _blackouts; - private IEnumerator _blackoutsEnum; + private readonly List _blackouts = new List(); + private readonly IEnumerator _blackoutsEnum; private int _nextFrameSwitch = -1; - public LightsSwitch(int episodeLength = 0, List blackouts = null) + public LightsSwitch() { - if (episodeLength < 0) - throw new ArgumentException("Episode length cannot be negative."); - - _episodeLength = episodeLength; - _blackouts = blackouts ?? new List(); + _blackoutsEnum = _blackouts.GetEnumerator(); + } - if (_blackouts.Count > 0) + public LightsSwitch(int episodeLength, List blackouts) + { + try { - foreach (var blackout in _blackouts) + if (episodeLength < 0) { - if (blackout < 0 || blackout >= _episodeLength) - throw new ArgumentException("Blackout interval is invalid."); + throw new ArgumentException("Episode length (timeLimit) cannot be negative."); } - _blackouts.Sort(); + _episodeLength = episodeLength; + _blackouts = blackouts ?? throw new ArgumentNullException(nameof(blackouts)); + + if (_blackouts.Count > 1) + { + for (int i = 1; i < _blackouts.Count; i++) + { + if (_blackouts[i] <= _blackouts[i - 1]) + { + throw new ArgumentException("Invalid blackout sequence: values must be in strictly increasing order."); + } + } + } - _blackoutsEnum = - _blackouts[0] < 0 - ? new InfiniteEnumerator(-_blackouts[0]) - : _blackouts.GetEnumerator(); + if (_blackouts.Count > 0 && _blackouts[0] < 0) + { + _blackoutsEnum = new InfiniteEnumerator(-_blackouts[0]); + } + else + { + _blackoutsEnum = _blackouts.GetEnumerator(); + } + Reset(); } - else + catch (Exception ex) { - _blackoutsEnum = _blackouts.GetEnumerator(); + Console.WriteLine($"Error initialzisn lightswitch: {ex.Message}"); + throw; } - - Reset(); } public void Reset() { - _lightStatus = true; - _blackoutsEnum.Reset(); - _nextFrameSwitch = _blackoutsEnum.MoveNext() ? _blackoutsEnum.Current : -1; + try + { + _lightStatus = true; + _blackoutsEnum.Reset(); + if (_blackoutsEnum.MoveNext()) + { + _nextFrameSwitch = _blackoutsEnum.Current; + } + else + { + _nextFrameSwitch = -1; + } + } + catch (Exception ex) + { + Console.WriteLine($"Error resetting LightsSwitch (blackouts): {ex.Message}"); + throw; + } } public bool LightStatus(int step, int agentDecisionInterval) { - if (step < 0 || agentDecisionInterval <= 0) - throw new ArgumentException("Step and agent decision interval must be positive."); - - if (step == _nextFrameSwitch * agentDecisionInterval) + try + { + if (step == _nextFrameSwitch * agentDecisionInterval) + { + _lightStatus = !_lightStatus; + if (_blackoutsEnum.MoveNext()) + { + _nextFrameSwitch = _blackoutsEnum.Current; + } + } + return _lightStatus; + } + catch (Exception ex) { - _lightStatus = !_lightStatus; - _nextFrameSwitch = _blackoutsEnum.MoveNext() ? _blackoutsEnum.Current : -1; + Console.WriteLine($"Error in LightStatus (blackouts): {ex.Message}"); + return _lightStatus; } - return _lightStatus; } } } From 2e438f186fa08fab41d17f213ce4fa2ad47c102d Mon Sep 17 00:00:00 2001 From: Ibrahim Alhas Date: Thu, 12 Sep 2024 16:20:15 +0100 Subject: [PATCH 2/6] typo fix --- Assets/Scripts/Lights.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Assets/Scripts/Lights.cs b/Assets/Scripts/Lights.cs index 41d924e..855f953 100644 --- a/Assets/Scripts/Lights.cs +++ b/Assets/Scripts/Lights.cs @@ -91,7 +91,7 @@ public LightsSwitch(int episodeLength, List blackouts) } catch (Exception ex) { - Console.WriteLine($"Error initialzisn lightswitch: {ex.Message}"); + Console.WriteLine($"Error initialising lightSwitch: {ex.Message}"); throw; } } From 4d0738f4f1ce618dc60ef2ae60f1405e6a76580b Mon Sep 17 00:00:00 2001 From: Ibrahim Alhas Date: Thu, 12 Sep 2024 19:01:20 +0100 Subject: [PATCH 3/6] updated lightswitchtests to be more robust and comprehensive --- Assets/Tests/EditMode/LightSwitchTests.cs | 92 +++++++++++++---------- 1 file changed, 54 insertions(+), 38 deletions(-) diff --git a/Assets/Tests/EditMode/LightSwitchTests.cs b/Assets/Tests/EditMode/LightSwitchTests.cs index 43fb828..f8d8269 100644 --- a/Assets/Tests/EditMode/LightSwitchTests.cs +++ b/Assets/Tests/EditMode/LightSwitchTests.cs @@ -9,15 +9,17 @@ public class LightsSwitchTests { [Test] - public void IncrementValueOnMoveNext() + public void InfiniteEnumerator_IncrementValueOnMoveNext() { - int initialValue = 3; - var enumerator = new InfiniteEnumerator(initialValue); - + var enumerator = new InfiniteEnumerator(3); enumerator.MoveNext(); - int currentValue = enumerator.Current; + Assert.AreEqual(3, enumerator.Current); + } - Assert.AreEqual(initialValue, currentValue); + [Test] + public void LightsSwitch_ThrowExceptionForNegativeEpisodeLength() + { + Assert.Throws(() => new LightsSwitch(-10, new List { 1, 2, 3 })); } [Test] @@ -34,61 +36,75 @@ public void ResetEnumeratorToZero() } [Test] - public void KeepIncrementingOnMultipleMoveNext() + public void InfiniteEnumerator_ResetToZero() { - int initialValue = 2; - var enumerator = new InfiniteEnumerator(initialValue); - - enumerator.MoveNext(); - Assert.AreEqual(2, enumerator.Current); - + var enumerator = new InfiniteEnumerator(5); enumerator.MoveNext(); - Assert.AreEqual(4, enumerator.Current); + enumerator.Reset(); + Assert.AreEqual(0, enumerator.Current); + } - enumerator.MoveNext(); - Assert.AreEqual(6, enumerator.Current); + [Test] + public void LightsSwitch_InitializeWithValidParameters() + { + var lightsSwitch = new LightsSwitch(10, new List { 1, 2, 3 }); + Assert.DoesNotThrow(() => lightsSwitch.Reset()); } [Test] - public void ThrowExceptionForNegativeEpisodeLength() + public void LightsSwitch_HandleInfiniteBlackouts() { - Assert.Throws(() => new LightsSwitch(-1, new List { 1 })); + var lightsSwitch = new LightsSwitch(10, new List { -2 }); + Assert.IsTrue(lightsSwitch.LightStatus(0, 1)); + Assert.IsFalse(lightsSwitch.LightStatus(2, 1)); + Assert.IsTrue(lightsSwitch.LightStatus(4, 1)); } [Test] - public void ThrowExceptionForInvalidBlackoutInterval() + public void LightsSwitch_ResetBehavior() { - Assert.Throws(() => new LightsSwitch(5, new List { -1, 6 })); + var lightsSwitch = new LightsSwitch(10, new List { 1, 3 }); + lightsSwitch.LightStatus(1, 1); + lightsSwitch.Reset(); + Assert.IsTrue(lightsSwitch.LightStatus(0, 1)); } [Test] - public void ReturnTrueIfNoBlackouts() + public void LightsSwitch_HandleEmptyBlackoutList() { var lightsSwitch = new LightsSwitch(10, new List()); - - bool lightStatus = lightsSwitch.LightStatus(0, 1); - - Assert.IsTrue(lightStatus); + Assert.IsTrue(lightsSwitch.LightStatus(0, 1)); + Assert.IsTrue(lightsSwitch.LightStatus(5, 1)); } [Test] - public void HandleMultipleBlackoutsProperly() + public void LightsSwitch_HandleDifferentAgentDecisionIntervals() { - var blackouts = new List { 1, 2, 3 }; - var lightsSwitch = new LightsSwitch(10, blackouts); - - Assert.IsTrue(lightsSwitch.LightStatus(0, 1)); - Assert.IsFalse(lightsSwitch.LightStatus(1, 1)); - Assert.IsTrue(lightsSwitch.LightStatus(2, 1)); - Assert.IsFalse(lightsSwitch.LightStatus(3, 1)); + var lightsSwitch = new LightsSwitch(20, new List { 2, 4 }); + Assert.IsTrue(lightsSwitch.LightStatus(0, 2)); + Assert.IsFalse(lightsSwitch.LightStatus(4, 2)); + Assert.IsTrue(lightsSwitch.LightStatus(8, 2)); } [Test] - public void ThrowExceptionForInvalidStepOrAgentDecisionInterval() + public void LightsSwitch_ThrowExceptionForInvalidBlackoutSequence() { - var lightsSwitch = new LightsSwitch(10, new List { 1 }); + Assert.Throws(() => new LightsSwitch(20, new List { 10, 1 })); + } - Assert.Throws(() => lightsSwitch.LightStatus(-1, 1)); - Assert.Throws(() => lightsSwitch.LightStatus(1, 0)); + [Test] + public void LightsSwitch_HandleInfiniteBlackoutsWithNegativeNumber() + { + var lightsSwitch = new LightsSwitch(100, new List { -20 }); + Assert.IsTrue(lightsSwitch.LightStatus(0, 1)); + Assert.IsTrue(lightsSwitch.LightStatus(19, 1)); + Assert.IsFalse(lightsSwitch.LightStatus(20, 1)); + Assert.IsFalse(lightsSwitch.LightStatus(39, 1)); + Assert.IsTrue(lightsSwitch.LightStatus(40, 1)); + Assert.IsTrue(lightsSwitch.LightStatus(59, 1)); + Assert.IsFalse(lightsSwitch.LightStatus(60, 1)); + Assert.IsFalse(lightsSwitch.LightStatus(79, 1)); + Assert.IsTrue(lightsSwitch.LightStatus(80, 1)); } -} + +} \ No newline at end of file From 770581bd37987f75db16b48a2dc8755e978ee647 Mon Sep 17 00:00:00 2001 From: Ibrahim Alhas Date: Tue, 17 Sep 2024 14:03:33 +0100 Subject: [PATCH 4/6] PR #42 review-related changes: - removed unnecessary exception handling. - removed explicit call to console.write to avoid unnecessary console logs The method is now more streamlined and clearer. --- Assets/Scripts/Lights.cs | 43 +++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/Assets/Scripts/Lights.cs b/Assets/Scripts/Lights.cs index 855f953..b54b061 100644 --- a/Assets/Scripts/Lights.cs +++ b/Assets/Scripts/Lights.cs @@ -58,42 +58,35 @@ public LightsSwitch() public LightsSwitch(int episodeLength, List blackouts) { - try + if (episodeLength < 0) { - if (episodeLength < 0) - { - throw new ArgumentException("Episode length (timeLimit) cannot be negative."); - } + throw new ArgumentException("Episode length (timeLimit) cannot be negative.", nameof(episodeLength)); + } - _episodeLength = episodeLength; - _blackouts = blackouts ?? throw new ArgumentNullException(nameof(blackouts)); + _episodeLength = episodeLength; + _blackouts = blackouts ?? throw new ArgumentNullException(nameof(blackouts)); - if (_blackouts.Count > 1) + if (_blackouts.Count > 1) + { + for (int i = 1; i < _blackouts.Count; i++) { - for (int i = 1; i < _blackouts.Count; i++) + if (_blackouts[i] <= _blackouts[i - 1]) { - if (_blackouts[i] <= _blackouts[i - 1]) - { - throw new ArgumentException("Invalid blackout sequence: values must be in strictly increasing order."); - } + throw new ArgumentException("Invalid blackout sequence: values must be in strictly increasing order.", nameof(blackouts)); } } + } - if (_blackouts.Count > 0 && _blackouts[0] < 0) - { - _blackoutsEnum = new InfiniteEnumerator(-_blackouts[0]); - } - else - { - _blackoutsEnum = _blackouts.GetEnumerator(); - } - Reset(); + if (_blackouts.Count > 0 && _blackouts[0] < 0) + { + _blackoutsEnum = new InfiniteEnumerator(-_blackouts[0]); } - catch (Exception ex) + else { - Console.WriteLine($"Error initialising lightSwitch: {ex.Message}"); - throw; + _blackoutsEnum = _blackouts.GetEnumerator(); } + + Reset(); } public void Reset() From cdca540b73e3e5e758f65d27967faf19fd5af99c Mon Sep 17 00:00:00 2001 From: Ibrahim Alhas Date: Tue, 17 Sep 2024 19:25:44 +0100 Subject: [PATCH 5/6] refactored test unit: LightsSwitch_HandleInfiniteBlackoutsWithNegativeNumber() Now cleaner and readable. --- Assets/Tests/EditMode/LightSwitchTests.cs | 34 +++++++++++++++-------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/Assets/Tests/EditMode/LightSwitchTests.cs b/Assets/Tests/EditMode/LightSwitchTests.cs index f8d8269..62145ef 100644 --- a/Assets/Tests/EditMode/LightSwitchTests.cs +++ b/Assets/Tests/EditMode/LightSwitchTests.cs @@ -95,16 +95,28 @@ public void LightsSwitch_ThrowExceptionForInvalidBlackoutSequence() [Test] public void LightsSwitch_HandleInfiniteBlackoutsWithNegativeNumber() { - var lightsSwitch = new LightsSwitch(100, new List { -20 }); - Assert.IsTrue(lightsSwitch.LightStatus(0, 1)); - Assert.IsTrue(lightsSwitch.LightStatus(19, 1)); - Assert.IsFalse(lightsSwitch.LightStatus(20, 1)); - Assert.IsFalse(lightsSwitch.LightStatus(39, 1)); - Assert.IsTrue(lightsSwitch.LightStatus(40, 1)); - Assert.IsTrue(lightsSwitch.LightStatus(59, 1)); - Assert.IsFalse(lightsSwitch.LightStatus(60, 1)); - Assert.IsFalse(lightsSwitch.LightStatus(79, 1)); - Assert.IsTrue(lightsSwitch.LightStatus(80, 1)); - } + /* + * The blackout list contains a single negative number, which means that the blackout will go repeat infinitely on the blackoutInterval frame. + * The light should be OFF at step 0, ON at step 1, OFF at step 2, and so on. + */ + const int episodeLength = 100; + const int blackoutInterval = 20; + const int agentDecisionInterval = 1; + + var blackoutList = new List { -blackoutInterval }; + var lightsSwitch = new LightsSwitch(episodeLength, blackoutList); + + /* Initial light status checks, then follows with the blackout sequences */ + Assert.IsTrue(lightsSwitch.LightStatus(0, agentDecisionInterval), "Light should be ON at step 0"); + Assert.IsTrue(lightsSwitch.LightStatus(19, agentDecisionInterval), "Light should be ON before blackout at step 19"); + Assert.IsFalse(lightsSwitch.LightStatus(20, agentDecisionInterval), "Light should be OFF during blackout at step 20"); + Assert.IsFalse(lightsSwitch.LightStatus(39, agentDecisionInterval), "Light should be OFF during blackout at step 39"); + + Assert.IsTrue(lightsSwitch.LightStatus(40, agentDecisionInterval), "Light should be ON after blackout at step 40"); + Assert.IsTrue(lightsSwitch.LightStatus(59, agentDecisionInterval), "Light should be ON before next blackout at step 59"); + + Assert.IsFalse(lightsSwitch.LightStatus(60, agentDecisionInterval), "Light should be OFF during blackout at step 60"); + Assert.IsFalse(lightsSwitch.LightStatus(79, agentDecisionInterval), "Light should be OFF during blackout at step 79"); + } } \ No newline at end of file From c986be32c5a98e7371db2e839b8a3909d85f6570 Mon Sep 17 00:00:00 2001 From: Ibrahim Alhas Date: Thu, 19 Sep 2024 13:07:05 +0100 Subject: [PATCH 6/6] updated validation on blackouts: Only throw if blackout > _episodeLength, as blackout == _episodeLength is acceptable. --- Edge case would be if: blackouts: [10, 100] -> meaning exactly equal to timeLimit (assuming timeLimit == 100) An invalid case would be: (blackouts: [10, 101] -> Invalid, as the blackout exceeds the timeLimit of 100.) --- Assets/Scripts/Lights.cs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/Assets/Scripts/Lights.cs b/Assets/Scripts/Lights.cs index b54b061..b2b8c5e 100644 --- a/Assets/Scripts/Lights.cs +++ b/Assets/Scripts/Lights.cs @@ -77,9 +77,21 @@ public LightsSwitch(int episodeLength, List blackouts) } } - if (_blackouts.Count > 0 && _blackouts[0] < 0) + if (_blackouts.Count > 0) { - _blackoutsEnum = new InfiniteEnumerator(-_blackouts[0]); + if (_blackouts[_blackouts.Count - 1] > _episodeLength) + { + throw new ArgumentException("Blackout time cannot exceed the episode length (timeLimit).", nameof(blackouts)); + } + + if (_blackouts[0] < 0) + { + _blackoutsEnum = new InfiniteEnumerator(-_blackouts[0]); + } + else + { + _blackoutsEnum = _blackouts.GetEnumerator(); + } } else {