Merge pull request #1712 from karashiiro/fix/gameversion-issues

Fix edge case in GameVersion instance creation and refactor

Dalamud.Common/Game/GameVersion.cs

@@ -23,7 +23,6 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
     /// Initializes a new instance of the <see cref="GameVersion"/> class.
     /// </summary>
     /// <param name="version">Version string to parse.</param>
-    [JsonConstructor]
     public GameVersion(string version)
     {
         var ver = Parse(version);
@@ -42,20 +41,9 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
     /// <param name="day">The day.</param>
     /// <param name="major">The major version.</param>
     /// <param name="minor">The minor version.</param>
-    public GameVersion(int year, int month, int day, int major, int minor)
+    [JsonConstructor]
+    public GameVersion(int year, int month, int day, int major, int minor) : this(year, month, day, major)
     {
-        if ((this.Year = year) < 0)
-            throw new ArgumentOutOfRangeException(nameof(year));
-
-        if ((this.Month = month) < 0)
-            throw new ArgumentOutOfRangeException(nameof(month));
-
-        if ((this.Day = day) < 0)
-            throw new ArgumentOutOfRangeException(nameof(day));
-
-        if ((this.Major = major) < 0)
-            throw new ArgumentOutOfRangeException(nameof(major));
-
         if ((this.Minor = minor) < 0)
             throw new ArgumentOutOfRangeException(nameof(minor));
     }
@@ -67,17 +55,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
     /// <param name="month">The month.</param>
     /// <param name="day">The day.</param>
     /// <param name="major">The major version.</param>
-    public GameVersion(int year, int month, int day, int major)
+    public GameVersion(int year, int month, int day, int major) : this(year, month, day)
     {
-        if ((this.Year = year) < 0)
-            throw new ArgumentOutOfRangeException(nameof(year));
-
-        if ((this.Month = month) < 0)
-            throw new ArgumentOutOfRangeException(nameof(month));
-
-        if ((this.Day = day) < 0)
-            throw new ArgumentOutOfRangeException(nameof(day));
-
         if ((this.Major = major) < 0)
             throw new ArgumentOutOfRangeException(nameof(major));
     }
@@ -88,14 +67,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
     /// <param name="year">The year.</param>
     /// <param name="month">The month.</param>
     /// <param name="day">The day.</param>
-    public GameVersion(int year, int month, int day)
+    public GameVersion(int year, int month, int day) : this(year, month)
     {
-        if ((this.Year = year) < 0)
-            throw new ArgumentOutOfRangeException(nameof(year));
-
-        if ((this.Month = month) < 0)
-            throw new ArgumentOutOfRangeException(nameof(month));
-
         if ((this.Day = day) < 0)
             throw new ArgumentOutOfRangeException(nameof(day));
     }
@@ -105,11 +78,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
     /// </summary>
     /// <param name="year">The year.</param>
     /// <param name="month">The month.</param>
-    public GameVersion(int year, int month)
+    public GameVersion(int year, int month) : this(year)
     {
-        if ((this.Year = year) < 0)
-            throw new ArgumentOutOfRangeException(nameof(year));
-
         if ((this.Month = month) < 0)
             throw new ArgumentOutOfRangeException(nameof(month));
     }
@@ -183,17 +153,13 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
 
     public static bool operator <(GameVersion v1, GameVersion v2)
     {
-        if (v1 is null)
+        ArgumentNullException.ThrowIfNull(v1);
-            throw new ArgumentNullException(nameof(v1));
-
         return v1.CompareTo(v2) < 0;
     }
 
     public static bool operator <=(GameVersion v1, GameVersion v2)
     {
-        if (v1 is null)
+        ArgumentNullException.ThrowIfNull(v1);
-            throw new ArgumentNullException(nameof(v1));
-
         return v1.CompareTo(v2) <= 0;
     }
 
@@ -209,8 +175,7 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
 
     public static GameVersion operator +(GameVersion v1, TimeSpan v2)
     {
-        if (v1 == null)
+        ArgumentNullException.ThrowIfNull(v1);
-            throw new ArgumentNullException(nameof(v1));
 
         if (v1.Year == -1 || v1.Month == -1 || v1.Day == -1)
             return v1;
@@ -222,8 +187,7 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
 
     public static GameVersion operator -(GameVersion v1, TimeSpan v2)
     {
-        if (v1 == null)
+        ArgumentNullException.ThrowIfNull(v1);
-            throw new ArgumentNullException(nameof(v1));
 
         if (v1.Year == -1 || v1.Month == -1 || v1.Day == -1)
             return v1;
@@ -240,18 +204,18 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
     /// <returns>GameVersion object.</returns>
     public static GameVersion Parse(string input)
     {
-        if (input == null)
+        ArgumentNullException.ThrowIfNull(input);
-            throw new ArgumentNullException(nameof(input));
 
         if (input.ToLower(CultureInfo.InvariantCulture) == "any")
-            return new GameVersion();
+            return Any;
 
         var parts = input.Split('.');
-        var tplParts = parts.Select(p =>
+        var tplParts = parts.Select(
-        {
+            p =>
-            var result = int.TryParse(p, out var value);
+            {
-            return (result, value);
+                var result = int.TryParse(p, out var value);
-        }).ToArray();
+                return (result, value);
+            }).ToArray();
 
         if (tplParts.Any(t => !t.result))
             throw new FormatException("Bad formatting");
@@ -259,18 +223,15 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
         var intParts = tplParts.Select(t => t.value).ToArray();
         var len = intParts.Length;
 
-        if (len == 1)
+        return len switch
-            return new GameVersion(intParts[0]);
+        {
-        else if (len == 2)
+            1 => new GameVersion(intParts[0]),
-            return new GameVersion(intParts[0], intParts[1]);
+            2 => new GameVersion(intParts[0], intParts[1]),
-        else if (len == 3)
+            3 => new GameVersion(intParts[0], intParts[1], intParts[2]),
-            return new GameVersion(intParts[0], intParts[1], intParts[2]);
+            4 => new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3]),
-        else if (len == 4)
+            5 => new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3], intParts[4]),
-            return new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3]);
+            _ => throw new ArgumentException("Too many parts"),
-        else if (len == 5)
+        };
-            return new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3], intParts[4]);
-        else
-            throw new ArgumentException("Too many parts");
     }
 
     /// <summary>
@@ -299,17 +260,12 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
     /// <inheritdoc/>
     public int CompareTo(object? obj)
     {
-        if (obj == null)
+        return obj switch
-            return 1;
-
-        if (obj is GameVersion value)
         {
-            return this.CompareTo(value);
+            null => 1,
-        }
+            GameVersion value => this.CompareTo(value),
-        else
+            _ => throw new ArgumentException("Argument must be a GameVersion", nameof(obj)),
-        {
+        };
-            throw new ArgumentException("Argument must be a GameVersion");
-        }
     }
 
     /// <inheritdoc/>
@@ -342,16 +298,14 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
         if (this.Minor != value.Minor)
             return this.Minor > value.Minor ? 1 : -1;
 
+        // This should never happen
         return 0;
     }
 
     /// <inheritdoc/>
     public override bool Equals(object? obj)
     {
-        if (obj is not GameVersion value)
+        return obj is GameVersion value && this.Equals(value);
-            return false;
-
-        return this.Equals(value);
     }
 
     /// <inheritdoc/>
@@ -373,16 +327,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
     /// <inheritdoc/>
     public override int GetHashCode()
     {
-        var accumulator = 0;
+        // https://learn.microsoft.com/en-us/dotnet/api/system.object.gethashcode?view=net-8.0#notes-to-inheritors
-
+        return HashCode.Combine(this.Year, this.Month, this.Day, this.Major, this.Minor);
-        // This might be horribly wrong, but it isn't used heavily.
-        accumulator |= this.Year.GetHashCode();
-        accumulator |= this.Month.GetHashCode();
-        accumulator |= this.Day.GetHashCode();
-        accumulator |= this.Major.GetHashCode();
-        accumulator |= this.Minor.GetHashCode();
-
-        return accumulator;
     }
 
     /// <inheritdoc/>
@@ -396,11 +342,11 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable<GameVersi
             return "any";
 
         return new StringBuilder()
-               .Append(string.Format("{0:D4}.", this.Year == -1 ? 0 : this.Year))
+               .Append($"{(this.Year == -1 ? 0 : this.Year):D4}.")
-               .Append(string.Format("{0:D2}.", this.Month == -1 ? 0 : this.Month))
+               .Append($"{(this.Month == -1 ? 0 : this.Month):D2}.")
-               .Append(string.Format("{0:D2}.", this.Day == -1 ? 0 : this.Day))
+               .Append($"{(this.Day == -1 ? 0 : this.Day):D2}.")
-               .Append(string.Format("{0:D4}.", this.Major == -1 ? 0 : this.Major))
+               .Append($"{(this.Major == -1 ? 0 : this.Major):D4}.")
-               .Append(string.Format("{0:D4}", this.Minor == -1 ? 0 : this.Minor))
+               .Append($"{(this.Minor == -1 ? 0 : this.Minor):D4}")
                .ToString();
     }
 }

Dalamud.Test/Dalamud.Test.csproj

@@ -5,7 +5,7 @@
         <RuntimeIdentifier>win-x64</RuntimeIdentifier>
         <PlatformTarget>x64</PlatformTarget>
         <Platforms>x64;AnyCPU</Platforms>
-        <LangVersion>9.0</LangVersion>
+        <LangVersion>11.0</LangVersion>
     </PropertyGroup>
 
     <PropertyGroup Label="Feature">

Dalamud.Test/Game/GameVersionTests.cs

@@ -1,10 +1,71 @@
+using System;
+
 using Dalamud.Common.Game;
+
+using Newtonsoft.Json;
+
 using Xunit;
 
 namespace Dalamud.Test.Game
 {
     public class GameVersionTests
     {
+        [Fact]
+        public void VersionComparisons()
+        {
+            var v1 = GameVersion.Parse("2021.01.01.0000.0000");
+            var v2 = GameVersion.Parse("2021.01.01.0000.0000");
+            Assert.True(v1 == v2);
+            Assert.False(v1 != v2);
+            Assert.False(v1 < v2);
+            Assert.True(v1 <= v2);
+            Assert.False(v1 > v2);
+            Assert.True(v1 >= v2);
+        }
+
+        [Fact]
+        public void VersionAddition()
+        {
+            var v1 = GameVersion.Parse("2021.01.01.0000.0000");
+            var v2 = GameVersion.Parse("2021.01.05.0000.0000");
+            Assert.Equal(v2, v1 + TimeSpan.FromDays(4));
+        }
+
+        [Fact]
+        public void VersionAdditionAny()
+        {
+            Assert.Equal(GameVersion.Any, GameVersion.Any + TimeSpan.FromDays(4));
+        }
+
+        [Fact]
+        public void VersionSubtraction()
+        {
+            var v1 = GameVersion.Parse("2021.01.05.0000.0000");
+            var v2 = GameVersion.Parse("2021.01.01.0000.0000");
+            Assert.Equal(v2, v1 - TimeSpan.FromDays(4));
+        }
+
+        [Fact]
+        public void VersionSubtractionAny()
+        {
+            Assert.Equal(GameVersion.Any, GameVersion.Any - TimeSpan.FromDays(4));
+        }
+
+        [Fact]
+        public void VersionClone()
+        {
+            var v1 = GameVersion.Parse("2021.01.01.0000.0000");
+            var v2 = v1.Clone();
+            Assert.NotSame(v1, v2);
+        }
+
+        [Fact]
+        public void VersionCast()
+        {
+            var v = GameVersion.Parse("2021.01.01.0000.0000");
+            Assert.Equal("2021.01.01.0000.0000", v);
+        }
+
         [Theory]
         [InlineData("any", "any")]
         [InlineData("2021.01.01.0000.0000", "2021.01.01.0000.0000")]
@@ -14,6 +75,18 @@ namespace Dalamud.Test.Game
             var v2 = GameVersion.Parse(ver2);
 
             Assert.Equal(v1, v2);
+            Assert.Equal(0, v1.CompareTo(v2));
+            Assert.Equal(v1.GetHashCode(), v2.GetHashCode());
+        }
+
+        [Fact]
+        public void VersionNullEquality()
+        {
+            // Tests `Equals(GameVersion? value)`
+            Assert.False(GameVersion.Parse("2021.01.01.0000.0000").Equals(null));
+
+            // Tests `Equals(object? value)`
+            Assert.False(GameVersion.Parse("2021.01.01.0000.0000").Equals((object)null));
         }
 
         [Theory]
@@ -31,6 +104,67 @@ namespace Dalamud.Test.Game
             Assert.True(v1.CompareTo(v2) < 0);
         }
 
+        [Theory]
+        [InlineData("any", "2020.06.15.0000.0000")]
+        public void VersionComparisonInverse(string ver1, string ver2)
+        {
+            var v1 = GameVersion.Parse(ver1);
+            var v2 = GameVersion.Parse(ver2);
+
+            Assert.True(v1.CompareTo(v2) > 0);
+        }
+
+        [Fact]
+        public void VersionComparisonNull()
+        {
+            var v = GameVersion.Parse("2020.06.15.0000.0000");
+
+            // Tests `CompareTo(GameVersion? value)`
+            Assert.True(v.CompareTo(null) > 0);
+
+            // Tests `CompareTo(object? value)`
+            Assert.True(v.CompareTo((object)null) > 0);
+        }
+
+        [Fact]
+        public void VersionComparisonBoxed()
+        {
+            var v1 = GameVersion.Parse("2020.06.15.0000.0000");
+            var v2 = GameVersion.Parse("2020.06.15.0000.0000");
+            Assert.Equal(0, v1.CompareTo((object)v2));
+        }
+
+        [Fact]
+        public void VersionComparisonBoxedInvalid()
+        {
+            var v = GameVersion.Parse("2020.06.15.0000.0000");
+            Assert.Throws<ArgumentException>(() => v.CompareTo(42));
+        }
+
+        [Theory]
+        [InlineData("2020.06.15.0000.0000")]
+        [InlineData("2021.01.01.0000")]
+        [InlineData("2021.01.01")]
+        [InlineData("2021.01")]
+        [InlineData("2021")]
+        public void VersionParse(string ver)
+        {
+            var v = GameVersion.Parse(ver);
+            Assert.NotNull(v);
+        }
+
+        [Theory]
+        [InlineData("2020.06.15.0000.0000")]
+        [InlineData("2021.01.01.0000")]
+        [InlineData("2021.01.01")]
+        [InlineData("2021.01")]
+        [InlineData("2021")]
+        public void VersionTryParse(string ver)
+        {
+            Assert.True(GameVersion.TryParse(ver, out var v));
+            Assert.NotNull(v);
+        }
+
         [Theory]
         [InlineData("2020.06.15.0000.0000")]
         [InlineData("2021.01.01.0000")]
@@ -39,9 +173,8 @@ namespace Dalamud.Test.Game
         [InlineData("2021")]
         public void VersionConstructor(string ver)
         {
-            var v = GameVersion.Parse(ver);
+            var v = new GameVersion(ver);
-
+            Assert.NotNull(v);
-            Assert.True(v != null);
         }
 
         [Theory]
@@ -54,5 +187,78 @@ namespace Dalamud.Test.Game
             Assert.False(result);
             Assert.Null(v);
         }
+
+        [Theory]
+        [InlineData("any", "any")]
+        [InlineData("2020.06.15.0000.0000", "2020.06.15.0000.0000")]
+        [InlineData("2021.01.01.0000", "2021.01.01.0000.0000")]
+        [InlineData("2021.01.01", "2021.01.01.0000.0000")]
+        [InlineData("2021.01", "2021.01.00.0000.0000")]
+        [InlineData("2021", "2021.00.00.0000.0000")]
+        public void VersionToString(string ver1, string ver2)
+        {
+            var v1 = GameVersion.Parse(ver1);
+            Assert.Equal(ver2, v1.ToString());
+        }
+
+        [Fact]
+        public void VersionIsSerializationSafe()
+        {
+            var v = GameVersion.Parse("2020.06.15.0000.0000");
+            var serialized = JsonConvert.SerializeObject(v);
+            var deserialized = JsonConvert.DeserializeObject<GameVersion>(serialized);
+            Assert.Equal(v, deserialized);
+        }
+
+        [Fact]
+        public void VersionInvalidDeserialization()
+        {
+            var serialized = """
+                             {
+                                "Year": -1,
+                                "Month": -1,
+                                "Day": -1,
+                                "Major": -1,
+                                "Minor": -1,
+                             }
+                             """;
+            Assert.Throws<ArgumentOutOfRangeException>(() => JsonConvert.DeserializeObject<GameVersion>(serialized));
+        }
+
+        [Fact]
+        public void VersionConstructorNegativeYear()
+        {
+            Assert.Throws<ArgumentOutOfRangeException>(() => new GameVersion(-2024));
+        }
+
+        [Fact]
+        public void VersionConstructorNegativeMonth()
+        {
+            Assert.Throws<ArgumentOutOfRangeException>(() => new GameVersion(2024, -3));
+        }
+
+        [Fact]
+        public void VersionConstructorNegativeDay()
+        {
+            Assert.Throws<ArgumentOutOfRangeException>(() => new GameVersion(2024, 3, -13));
+        }
+
+        [Fact]
+        public void VersionConstructorNegativeMajor()
+        {
+            Assert.Throws<ArgumentOutOfRangeException>(() => new GameVersion(2024, 3, 13, -1));
+        }
+
+        [Fact]
+        public void VersionConstructorNegativeMinor()
+        {
+            Assert.Throws<ArgumentOutOfRangeException>(() => new GameVersion(2024, 3, 13, 0, -1));
+        }
+
+        [Fact]
+        public void VersionParseNull()
+        {
+            Assert.Throws<ArgumentNullException>(() => GameVersion.Parse(null!));
+        }
     }
 }