Improvement #42

ammarheidari · 2024-11-10T09:26:56Z

Syntax improvement and code cleanup

jshergal

The intention of the code changes are good, but when committing to an open source project, please keep in mind that there is an existing code style/standard. Much of this PR is modifying the code away from the existing standard (primarily with the change to use explicit variable type names in place of the exiting usage of var).

jshergal · 2024-11-16T14:05:04Z

src/Standard.Licensing.Tests/KeyGeneratorTests.cs

+            string passPhrase = "test";
+            HashSet<string> privKeySet = new HashSet<string>();
+            HashSet<string> pubKeySet = new HashSet<string>();


Why would you change all of the var lines to explicit type declarations? That is not a code cleanup, but a change in the overall standard of how the code is written within this project.

jshergal · 2024-11-16T14:05:19Z

src/Standard.Licensing.Tests/KeyGeneratorTests.cs

+                KeyGenerator keyGenerator = new KeyGenerator(256); //default key size
+                KeyPair? pair = keyGenerator.GenerateKeyPair();
+                string? privateKey = pair.ToEncryptedPrivateKeyString(passPhrase);
+                string? publicKey = pair.ToPublicKeyString();


Same thing here

In C#, the ? after a type denotes a nullable value type. This means that the variable can hold either a value of that type or null. This is particularly useful for value types (like int, double, bool, etc.) which cannot naturally be null.
Here are some reasons why you might want to use a nullable type:

Representing Absence of Value: It allows you to represent the absence of a value in a way that's consistent with how reference types work (where null can be used). For example, int? can represent an integer that might not have a value.

Default Values: For value types, the default value is generally 0, false, or another default. A nullable type allows you to explicitly indicate that the value is not set or unknown by setting it to null.

Database Interactions: When working with databases, many fields can have null values. Using nullable types ensures your C# code can accurately represent and handle these scenarios.

Conditional Logic: You can easily check if a nullable type has a value using .HasValue and retrieve the value using .Value.

Enhanced Readability and Maintenance: By explicitly declaring a type as nullable, it makes the code more readable and maintainable, clearly indicating that the variable is allowed to have no value.

@ammarheidari yes, this is the syntax for a nullable type, but the along with the change to coding style here, you changed the nullability of the values. For instance GenerateKeyPair() does not return a nullable value, so along with teh style change, this is a behavioral change.

jshergal · 2024-11-16T14:05:30Z

src/Standard.Licensing.Tests/LicenseSignatureTests.cs

+            KeyGenerator? keyGenerator = KeyGenerator.Create();
+            KeyPair? keyPair = keyGenerator.GenerateKeyPair();


jshergal · 2024-11-16T14:05:42Z

src/Standard.Licensing.Tests/LicenseSignatureTests.cs

+            License? license = License.New()
+                                      .CreateAndSignWithPrivateKey(privateKey, passPhrase);


jshergal · 2024-11-16T14:05:51Z

src/Standard.Licensing.Tests/LicenseSignatureTests.cs


            Assert.That(license, Is.Not.Null);
            Assert.That(license.Signature, Is.Not.Null);

            // validate xml
-            var xmlElement = XElement.Parse(license.ToString(), LoadOptions.None);
+            XElement xmlElement = XElement.Parse(license.ToString(), LoadOptions.None);


jshergal · 2024-11-16T14:39:04Z

src/Standard.Licensing/Validation/LicenseValidationExtensions.cs


-#else
+                #else


spacing of the preprocessor directive

jshergal · 2024-11-16T14:39:16Z

src/Standard.Licensing/Validation/LicenseValidationExtensions.cs


            validator.Validate = license => assemblies.All(
                    asm =>
                    asm.GetCustomAttributes<AssemblyBuildDateAttribute>()
                       .Cast<AssemblyBuildDateAttribute>()
                       .All(a => a.BuildDate < license.Expiration));

-#endif
+                #endif


spacing of the preprocessor directive

jshergal · 2024-11-16T14:39:47Z

src/Standard.Licensing/Validation/LicenseValidationExtensions.cs

@@ -125,11 +133,15 @@ public static IValidationChain ProductBuildDate(this IStartValidationChain valid
        /// <returns>An instance of <see cref="IStartValidationChain"/>.</returns>
        public static IValidationChain AssertThat(this IStartValidationChain validationChain, Predicate<License> predicate, IValidationFailure failure)
        {
-            var validationChainBuilder = (validationChain as ValidationChainBuilder);
-            var validator = validationChainBuilder.StartValidatorChain();
+            ValidationChainBuilder validationChainBuilder = validationChain as ValidationChainBuilder;


change from var

jshergal · 2024-11-16T14:39:56Z

src/Standard.Licensing/Validation/LicenseValidationExtensions.cs

-            var validationChainBuilder = (validationChain as ValidationChainBuilder);
-            var validator = validationChainBuilder.StartValidatorChain();
-            validator.Validate = license => license.VerifySignature(publicKey);
+            ValidationChainBuilder validationChainBuilder = validationChain as ValidationChainBuilder;


change from var

jshergal · 2024-11-16T14:42:18Z

src/Standard.Licensing/Validation/ValidationChainBuilder.cs

@@ -70,17 +73,13 @@ public IEnumerable<IValidationFailure> AssertValidLicense()
        {
            CompleteValidatorChain();

-            foreach (var validator in validators)
+            foreach (ILicenseValidator validator in validators.Where(validator => validator.ValidateWhen == null || validator.ValidateWhen(license)).Where(validator => !validator.Validate(license)))


change from var

ammarheidari · 2024-11-17T12:43:02Z

Changing var lines to explicit type declarations in C# can be beneficial for several reasons. Let's explore why:

Readability: Explicit type declarations make it clear what type a variable is, which can make the code easier to read and understand, especially for new developers or those not familiar with the codebase.
Clarity: In complex codebases, knowing the exact type of a variable without having to infer it can reduce confusion and errors.
IntelliSense and IDE Support: Explicit types can improve the accuracy and usefulness of IntelliSense in Visual Studio or other IDEs, making development smoother and more efficient.
Type Safety: Explicitly declaring types helps catch type-related errors at compile-time, which can be crucial for avoiding runtime errors.
Code Maintenance: Explicit type declarations can make the code easier to maintain and refactor, as the types are clearly stated, reducing the likelihood of introducing bugs during changes.
Documentation: Having explicit types can serve as a form of documentation, making it clear to others (and your future self) what kind of data structures and types are being used.

jshergal · 2024-11-17T14:57:37Z

To be honest, the var not var argument borders on being a religious argument amongst developers. I personally lean much more towards the use of var everywhere, as the underlying type should be clear from the naming and usage. I also make extensive use of auto in my C++ code for the same reason. At the same time, I can also agree with the Dotnet teams coding guidelines, which say to use var when the type is explicitly named on the right-hand side. To me, that feels like a decent compromise.

My bigger point here is that there is already an established coding style in the project and this PR is mainly about changing that style. There are advantages and disadvantages to both styles, and both sides have their adherents. So the point of my comments was not that "this style is wrong", but rather that the existing code (and the project that it is based upon) follow the style of "use var when possible".

If you would like to raise it as an issue and have the community discuss it, that would be great, and if others in the project feel like the change would be welcome, then the style could be changed, but it is something that the community should decide.

ammarheidari · 2024-11-18T08:46:05Z

Thanks for the follow-up and response. This was a suggestion for improvement. And whatever decision the community makes will be the same.

jshergal · 2024-11-18T17:49:38Z

@ammarheidari
It is great that you want to contribute and help the project. Please file an issue under the issues tab to suggest (along with your arguments as mentioned here) why it might be good to change the coding style of the project. That is the place that discussions will occur and decisions will be made.

ammarheidari · 2024-12-15T05:36:04Z

@jshergal
Any update on merge ?

jshergal · 2024-12-15T06:03:04Z

@ammarheidari - I am not a maintainer of the project and thus have no rights to merge PRs. I was simply providing some feedback.

Improvement

d5fbf9e

Syntax improvement and code cleanup

jshergal reviewed Nov 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvement #42

Improvement #42

ammarheidari commented Nov 10, 2024

jshergal left a comment

jshergal Nov 16, 2024

jshergal Nov 16, 2024

ammarheidari Nov 17, 2024

jshergal Nov 17, 2024

jshergal Nov 16, 2024

jshergal Nov 16, 2024

jshergal Nov 16, 2024

jshergal Nov 16, 2024

jshergal Nov 16, 2024

jshergal Nov 16, 2024

jshergal Nov 16, 2024

jshergal Nov 16, 2024

ammarheidari commented Nov 17, 2024

jshergal commented Nov 17, 2024

ammarheidari commented Nov 18, 2024

jshergal commented Nov 18, 2024

ammarheidari commented Dec 15, 2024

jshergal commented Dec 15, 2024

		KeyGenerator? keyGenerator = KeyGenerator.Create();
		KeyPair? keyPair = keyGenerator.GenerateKeyPair();

		License? license = License.New()
		.CreateAndSignWithPrivateKey(privateKey, passPhrase);

Improvement #42

Are you sure you want to change the base?

Improvement #42

Conversation

ammarheidari commented Nov 10, 2024

jshergal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ammarheidari commented Nov 17, 2024

jshergal commented Nov 17, 2024

ammarheidari commented Nov 18, 2024

jshergal commented Nov 18, 2024

ammarheidari commented Dec 15, 2024

jshergal commented Dec 15, 2024